fix: hash a hunk past the scanner's line limit - #197
Open
asyncawaitpromise wants to merge 6 commits into
Open
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces the use of bufio.Scanner with a manual byte-splitting loop in the hunkHash function in internal/git/diff.go. This change addresses a limitation where bufio.Scanner stops reading after its 64KB token limit, which could cause different hunks exceeding this size to produce identical hashes. A corresponding unit test TestHunkHashReadsLinesPastScannerLimit has been added to verify correct hashing behavior for over-long hunks, context lines, and CRLF line endings. There are no review comments, so I have no feedback to provide.
asyncawaitpromise
force-pushed
the
fix/hunk-hash-long-lines
branch
from
September 1, 2026 06:49
6f6fd79 to
fc53ef5
Compare
bufio.Scanner refuses a token longer than 64KB. hunkHash never checked Err(), so on a longer line it hashed only what it had read and stopped. Two hunks differing solely beyond that point therefore hashed alike, and a hunk matching the approval-time diff is dropped as already reviewed -- so an approval could survive a change nobody saw. hunkBlocks does check Err() and declines the hunk, which is why this only shows up here. Split the body by hand instead. There is no limit to exceed, so the failure mode is gone rather than pushed further out, and trailing carriage returns are dropped so a CRLF file hashes like any other. Coverage badge regenerated.
asyncawaitpromise
force-pushed
the
fix/hunk-hash-long-lines
branch
from
September 1, 2026 07:25
fc53ef5 to
682083c
Compare
asyncawaitpromise
marked this pull request as ready for review
September 1, 2026 07:31
|
Codeowners approval required for this PR: |
 The PR appears safe to merge. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Review catch: the carriage return was stripped from every line, including an unterminated final line where the CR is part of the content rather than a terminator. A hunk ending "+value\r" therefore hashed the same as one ending "+value", so it could be subtracted as already reviewed. That is the same fail-open direction this PR set out to close. Strip the CR only where it precedes a newline. A CRLF file still hashes like its LF twin, and the new test goes red without the change.
Review catch, and the same fail-open class as the rest of this PR. The kept lines were concatenated with nothing between them, so two lines hashed as their joined form: "+total = x\n+y\n" == "+total = x+y\n" "+foo\n-bar\n" == "+foo-bar\n" Any added line containing a plus or minus collides with the two-line hunk split at that character, and the author controls both sides of the diff. Write a separator after each line. Two more while in here. The "\ No newline at end of file" marker was skipped as a context line, so a hunk that only gained a trailing newline hashed into the approval set; it now counts. And the accumulation buffer had no bound once the scanner went away, so the lines stream into the digest instead: 1 allocation rather than a buffer that grows with the hunk. The two standalone tests are now rows in the existing table, which already had the right shape, and the three collisions above are rows too. Each new row fails without its fix.
|
Codeowners approval required for this PR: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary / Background
hunkHashis how we decide whether a reviewer has already seen a hunk: drop the context lines, keep the+and-lines, hash them. Every approval-time hunk goes into a set, and a current hunk whose hash is in that set gets dropped as already reviewed.It read the body with
bufio.Scanner, which refuses a token past 64KB. Scanner stops at the first over-long line and reports it throughErr(), which we never checked, so the hash only covered the lines read before that point.Two hunks that differ only past that point therefore hash alike. If one of them is in the approval-time diff, the other gets subtracted away and an approval survives a change nobody looked at. It fails open, which is the direction we least want.
hunkBlocksdoes checkErr()and declines the hunk, so this only shows up in the hash.Lines that long are not exotic. Minified bundles, generated clients, vendored single-line payloads and large fixtures all carry them.
Code Changes
Split the body by hand instead of scanning it. There is no limit left to exceed, so the failure mode is gone rather than pushed further out, and it drops an import rather than adding a knob.
Trailing carriage returns go too, so a CRLF file hashes like its LF twin instead of every line differing.
Tests cover the collision, the identical over-long pair, context lines staying out, and CRLF.