Skip to content

GitFilePatch.PatchFor on a subset of hunks keeps stale +new offsets, so git apply can stage a hunk at the wrong place and still exit 0 #167

Description

@matt-edmondson

What's wrong

GitFilePatch.PatchFor (GitIntegration/Models/GitPatch.cs:148-167) writes each chosen hunk's Text exactly as git diff produced it, @@ -oldStart,oldCount +newStart,newCount @@ header included. The +newStart in that header counts the lines added and removed by every earlier hunk in the diff. When PatchFor leaves an earlier hunk out, a later hunk's +newStart points past where the change really belongs.

git apply uses that position as the place to start searching for the hunk's context, and it searches outward from there. If the same context also appears somewhere in the file closer to the stale position, git applies the hunk at that other place. It reports success and prints no warning.

Reproduction (git 2.43)

  1. Commit a 120-line file that has an identical 7-line block at lines 50-56 and again at lines 81-87.
  2. In the working tree, insert 20 lines at line 5 (hunk 1) and change line 53 inside the first block (hunk 2).
  3. patch.PatchFor([patch.Hunks[1]]) produces @@ -50,7 +70,7 @@.
  4. git apply --cached on that text exits 0, but git diff --cached shows the change was staged at line 84, inside the second copy of the block. Line 53 is untouched.

The same text passed to Apply(...).ToIndex().Reversed() (unstage one hunk) has the same exposure. This is the core "stage one hunk" workflow the patch verbs exist for, and it can put a silently wrong edit into the index.

Why the tests miss it

The round-trip tests in GitIntegration.Test/Integration/GitPatchRoundTripTests.cs only ever stage Hunks[0], or all hunks together. In both cases the +newStart values are already correct. No test stages a later hunk without the earlier ones, and none uses repeated context.

Suggested fix

Rewrite each emitted hunk's header in PatchFor, the way git add -p recalculates headers when hunks are skipped:

  • Keep a running delta = Σ(newCount − oldCount) over the hunks already emitted.
  • Emit each hunk with +newStart = oldStart + delta, leaving -oldStart,oldCount and newCount unchanged.
  • GitHunk already exposes the parsed start and count values, so only the header line of Text needs replacing.

Acceptance criteria

  • An integration test builds the scenario above, stages only Hunks[1], and asserts the index changed line 53 and not line 84.
  • A matching test unstages a single later hunk with .Reversed().
  • The existing round-trip tests still pass.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions