-
-
Notifications
You must be signed in to change notification settings - Fork 954
Ignore all lines of subsequent hunks until last one is found #602
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
Conversation
Git version 2.11.1+ introduced extra lines into the subsequent hunk sections for incremental blame output. The documentation notes that parsers of this output should ignore all lines between the start and end for robust parsing.
Codecov Report
@@ Coverage Diff @@ ## master #602 +/- ## ========================================= + Coverage 94.48% 94.5% +0.02% ========================================= Files 63 63 Lines 9958 9961 +3 ========================================= + Hits 9409 9414 +5 + Misses 549 547 -2
Continue to review full report at Codecov.
|
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.
I believe this loops break-condition is not clearly defined just now.
Do you think 'end-of-stream' could be another one?
What do you think of something like this:
for line in stream: tag, value = line.split(b' ', 1) if tag == b'filename': orig_filename = value break # Finally handle the case of the stream being exhausted too early
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.
I've added a comment in 77b20be to be more explicit about an unexpected EOF condition. With a for loop we'd need to raise that exception ourselves in an else block so I think the while loop works better here?
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.
Ah, I see now! I didn't know that next throws - iterators do that, right. Have been doing Rust for too long ;).
Thanks a lot! In addition to the comment, how feasible/easy is it for you to add a test based on the kind of input you are handling? I believe there are fixtures for this already, maybe it's possible to adjust one too.
@Byron – yep I think that sort of test should be easy to add with an extra fixture that has the new style git output. I'll add that as soon as I get the time :)
@ghickman Great, I will be waiting for that one then - by now github sends emails on new commits, so I should see it and merge shortly thereafter.
41b7fac to
73a5926
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 a lot! I am ready to merge!
One last question though: should there or should there not be empty lines in the fixture?
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.
I don't think there should be empty lines in the fixture.
If it helps I generated the fixture with git blame --incremental -p 82b8902e033430000481eb355733cd7065342037 -- AUTHORS > git/test/fixtures/blame_incremental_2.11.1_plus (which I believe matches how blame_incremental was generated).
Awesome! Thanks a lot for this one!
Git version 2.11.1+ introduced extra lines into the subsequent hunk
sections for incremental blame output. The documentation notes that
parsers of this output should ignore all lines between the start and end
for robust parsing.