Skip to content

Add it blame-copy-royal #2041

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Jun 24, 2025
Merged

Conversation

cruessler
Copy link
Contributor

This is a draft PR. It is mostly intended for early feedback, in case there’s any. I plan on addressing the remaining TODOs over the course of the coming days. I also plan on providing more context. :-)

@Byron
Copy link
Member

Byron commented Jun 9, 2025

Thanks for making this happen!

Early feedback could be that this seems to be a breaking change in gix-blame, which should then go into a separate commit. I didn't look at it beyond that though.

@cruessler cruessler force-pushed the add-blame-extraction branch 3 times, most recently from 039d300 to 6c6c733 Compare June 15, 2025 11:10
@cruessler cruessler force-pushed the add-blame-extraction branch from 6c6c733 to 01cd9bd Compare June 15, 2025 14:09
@cruessler cruessler force-pushed the add-blame-extraction branch from 01cd9bd to b01d624 Compare June 15, 2025 16:43
@cruessler cruessler marked this pull request as ready for review June 20, 2025 06:52
@cruessler
Copy link
Contributor Author

@Byron I think I’ve now reached a point where it makes sense for you to start reviewing!

I tried to add as much context as possible in comments. I hope that’s enough context, let me know if you need more! I also tried to have each commit only touch a single crate. Feel free to squash if you want!

What I wasn’t sure about was the command’s name. This initial version basically just copies the name from copy-royal because it seems somehow related. Another option that might work is extract-blame-history.

I will continue testing the PR in the background. My gut feeling tells me that, at this point, any issues I might still find could also be issues in the blame implementation itself.

Byron added 4 commits June 24, 2025 08:37
Also provide more context in comment
* rename `index` to `parent_index`.
* add test for clap definition of internal-tools
* optimize generated script a bit
With latest nightly
@Byron Byron force-pushed the add-blame-extraction branch from dff475d to 554ce13 Compare June 24, 2025 07:27
Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much, I am loving it! I think it can also be 'abused' to create histories for all commits that touch a certain file, independently of what it is used for later.

This will certainly be useful for debugging in other contexts.
Despite the very well documented 'shortcomings' in the general case as it only sees commits relevant to the blame algorithm, and not all that actually touched a file, it's definitely a step up to what was available before.

Performance of the generated script

The generated script I thought I could get a little bit faster, with no results worth bragging about.

The initial run of cargo run -p internal-tools --release -- blame-copy-royal . out README.md took 38s, and by the end it was … 37s :D.
I tried to remove the tagging and replace with checking out commits… until I realised my mistake :D.

Finally, there is just one comment left, but in case that should be addressed that can definitely happen in another PR.

set -e

git init
echo .gitignore >> .gitignore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This .gitignore rule I don't understand at all.
From what I see, the .gitignore file could be skipped entirely. What am I missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what remember, I ran into a situation where .gitignore showed up as an untracked file when I was debugging the target directory. Feel free to remove in case git ignores .gitignore by default or in case you want to remove some noise!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, it's fine :). It can still be removed if there are any issues with it, i.e. it's in the way of supporting debugging.

@Byron Byron enabled auto-merge June 24, 2025 07:28
@cruessler
Copy link
Contributor Author

The generated script I thought I could get a little bit faster, with no results worth bragging about.

The initial run of cargo run -p internal-tools --release -- blame-copy-royal . out README.md took 38s, and by the end it was … 37s :D. I tried to remove the tagging and replace with checking out commits… until I realised my mistake :D.

You’re right, the script hasn’t really been optimized for performance. :-) But the intermediate representation Vec<BlameScriptOperation> is supposed to make certain optimizations easier. For example, you can remove all instances of CheckoutTag(x) that immediately follow CreateTag(x). You can also remove all CreateTag(x) that are only ever referenced in a single CheckoutTag(x) that immediately follows the corresponding CreateTag(x). I didn’t do this right away as I wanted to wait for the results of your review first. I also think this is something that could be done in a follow-up commit.

@Byron Byron merged commit dd5f0a4 into GitoxideLabs:main Jun 24, 2025
23 checks passed
@Byron
Copy link
Member

Byron commented Jun 24, 2025

I also think this is something that could be done in a follow-up commit.

Only if it's fun for you :)! Performance doesn't matter too much there, because it did we'd serialise the data as RON and restore using gix entirely. It would be done in a second or less, without trying even.

@cruessler
Copy link
Contributor Author

Only if it's fun for you :)! Performance doesn't matter too much there, because it did we'd serialise the data as RON and restore using gix entirely. It would be done in a second or less, without trying even.

I was already starting to wonder why we’re using a shell script at all. 😄 Is it because we want git to be involved to make sure the created repository can serve as a baseline?

@Byron
Copy link
Member

Byron commented Jun 24, 2025

No, the shell script is needed to easily integrate them into the current test-system.
However, if ever needed, gix-testtools could of course learn how to restore such states more efficiently.

Meanwhile, if you feel your testing is slowed down by the script, it's no problem to implement a Rust version in addition to what's currently there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants