Uh oh!
There was an error while loading. Please reload this page.
Ignore all lines of subsequent hunks until last one is found - #602
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.
|
| orig_filename = value | ||
| # Discard all lines until we find "filename" which is | ||
| # guaranteed to be the last line | ||
| while True: |
There was a problem hiding this comment.
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:
forlineinstream:
tag, value=line.split(b' ', 1)
iftag==b'filename':
orig_filename=valuebreak# Finally handle the case of the stream being exhausted too earlyThere was a problem hiding this comment.
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.
Ah, I see now! I didn't know that next throws - iterators do that, right. Have been doing Rust for too long ;).
Byron
commented
Mar 7, 2017
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. |
ghickman
commented
Mar 7, 2017
@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 :) |
Byron
commented
Mar 7, 2017
@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. |
| @@ -0,0 +1,33 @@ | |||
| 82b8902e033430000481eb355733cd7065342037 2 2 1 | |||
There was a problem hiding this comment.
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.
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).
Byron
commented
Mar 7, 2017
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.