Fix applying patches with zero-context fragments#81
Open
gaoflow wants to merge 1 commit into
Open
Conversation
In unified diff format a range with a length of zero is positional: the hunk "@@ -3,0 +4 @@" inserts after old line 3 rather than starting at it. The text applier derived the start of every fragment from OldPosition the same way, so each zero-context insertion was applied one line too early. Apply returned nil in that case, so the result was silently corrupted rather than reported as an error. Two nearby checks read file-level facts off the same ambiguous positions. An OldPosition of 0 was taken to mean "create a file", but "@@ -0,0 +1 @@" is also how a patch inserts before the first line of an existing file. A new position of "+0,0" was taken to mean "delete the file", but "@@ -1 +0,0 @@" is also how a patch deletes only the first line. Both rejected patches that git apply --unidiff-zero and GNU patch accept. Start a fragment at OldPosition when it has no old lines, and move the creation and deletion checks to Apply, where File.IsNew and File.IsDelete are available. This matches ParseTextFragments, which already uses those fields rather than fragment positions to decide the same questions. text_fragment_error_new_file.patch is not a new file patch: it has no /dev/null side and no new file mode line, so git and GNU patch both apply it as an insertion before line 1. Replace it with a file-level test that uses a real new file patch, which still reports a conflict.
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.
Zero-context (
-U0) insertions are applied one line too early, andApplyreturns nil, so the output is silently wrong rather than an error:A zero-length range in a hunk header is positional —
@@ -3,0 +4 @@inserts after old line 3 — but the applier starts every fragment atOldPosition - 1. Two nearby checks read file-level facts off the same ambiguous positions and reject patches thatgit apply --unidiff-zeroand GNUpatchboth accept:@@ -0,0 +1 @@(insert before line 1) is read as file creation, and@@ -1 +0,0 @@(delete line 1) as a full delete. The fix starts a fragment atOldPositionwhen it has no old lines, and moves those two checks intoApplybehindFile.IsNew/File.IsDelete— the fieldsParseTextFragmentsalready uses to answer the same questions.Tested by diffing against
git apply --unidiff-zeroand GNUpatchover ~9,700 generated patches (-U0–-U3, insert/delete/replace at file start, middle and end, multi-hunk, empty files, no trailing newline, CRLF, and theFile.String()round-trip): 162 wrong results at-U0before, 0 after,-U1–-U3unchanged. One existing expectation changes —text_fragment_error_new_file.patchis not a new-file patch (no/dev/nullside, nonew file mode), and git and GNU patch both apply it as an insertion before line 1, so it is replaced by a file-level test using a real new-file patch that still reports a conflict.