-
-
Notifications
You must be signed in to change notification settings - Fork 355
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
Add it blame-copy-royal
#2041
Conversation
Thanks for making this happen! Early feedback could be that this seems to be a breaking change in |
039d300
to
6c6c733
Compare
6c6c733
to
01cd9bd
Compare
01cd9bd
to
b01d624
Compare
@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 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. |
Also provide more context in comment
With latest nightly
dff475d
to
554ce13
Compare
There was a problem hiding this 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
You’re right, the script hasn’t really been optimized for performance. :-) But the intermediate representation |
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 |
I was already starting to wonder why we’re using a shell script at all. 😄 Is it because we want |
No, the shell script is needed to easily integrate them into the current test-system. 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. |
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. :-)