From 0da2c370082b41e10766b7383e0b3f53169ca4b7 Mon Sep 17 00:00:00 2001 From: youdie006 Date: Thu, 10 Sep 2026 14:00:05 +0900 Subject: [PATCH 1/2] Reverse the extended headers in ReverseFileDiff ReverseFileDiff swaps OrigName/NewName, OrigTime/NewTime and every hunk range, but copied Extended verbatim. A reversed file creation therefore kept "new file mode 100644" while its +++ line became /dev/null, and git apply rejects that: error: git apply: bad git-diff - expected /dev/null on line 2 For an empty new file it is worse: PrintFileDiff short-circuits when Hunks is nil, so the reversed diff is byte-identical to the forward one and re-parsing gives back the original direction. Swap the direction-bearing headers the way parse.go already reads them: new file mode/deleted file mode, old mode/new mode, rename from/rename to, copy from/copy to, and the two hashes in the index line. Gated on a leading "diff --git " line, which is the same gate handleEmpty uses. --- diff/reverse.go | 66 +++++++++++++++++++++++++++++++- diff/reverse_test.go | 89 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 154 insertions(+), 1 deletion(-) diff --git a/diff/reverse.go b/diff/reverse.go index 87715ef..924b6b4 100644 --- a/diff/reverse.go +++ b/diff/reverse.go @@ -4,6 +4,7 @@ import ( "bytes" "errors" "fmt" + "strings" ) // ReverseFileDiff takes a diff.FileDiff, and returns the reverse operation. @@ -14,7 +15,7 @@ func ReverseFileDiff(fd *FileDiff) (*FileDiff, error) { OrigTime: fd.NewTime, NewName: fd.OrigName, NewTime: fd.OrigTime, - Extended: fd.Extended, + Extended: reverseExtendedHeaders(fd.Extended), } for _, hunk := range fd.Hunks { invHunk, err := reverseHunk(hunk) @@ -26,6 +27,69 @@ func ReverseFileDiff(fd *FileDiff) (*FileDiff, error) { return &reverse, nil } +// reverseExtendedHeaders reverses the direction encoded in git's extended +// header lines, matching what "git diff -R" emits. +func reverseExtendedHeaders(headers []string) []string { + // handleEmpty gates on the same prefix when it reads the direction back out. + if len(headers) == 0 || !strings.HasPrefix(headers[0], "diff --git ") { + return headers + } + reversed := make([]string, len(headers)) + copy(reversed, headers) + for i, header := range reversed { + switch { + case strings.HasPrefix(header, "new file mode "): + reversed[i] = "deleted file mode " + header[len("new file mode "):] + case strings.HasPrefix(header, "deleted file mode "): + reversed[i] = "new file mode " + header[len("deleted file mode "):] + case strings.HasPrefix(header, "index "): + reversed[i] = reverseIndexHeader(header) + } + } + swapHeaderValues(reversed, "old mode ", "new mode ") + swapHeaderValues(reversed, "rename from ", "rename to ") + swapHeaderValues(reversed, "copy from ", "copy to ") + return reversed +} + +// swapHeaderValues exchanges the values of the first "from" header and the +// first "to" header, leaving both prefixes where they are. +func swapHeaderValues(headers []string, fromPrefix, toPrefix string) { + from, to := -1, -1 + for i, header := range headers { + if from < 0 && strings.HasPrefix(header, fromPrefix) { + from = i + } + if to < 0 && strings.HasPrefix(header, toPrefix) { + to = i + } + } + if from < 0 || to < 0 { + return + } + headers[from], headers[to] = fromPrefix+headers[to][len(toPrefix):], toPrefix+headers[from][len(fromPrefix):] +} + +// reverseIndexHeader swaps the two blob hashes in an "index ..[ ]" +// header, leaving the trailing mode (if any) alone. +func reverseIndexHeader(header string) string { + const prefix = "index " + rest := header[len(prefix):] + var suffix string + if strings.HasSuffix(rest, "\r") { + rest, suffix = rest[:len(rest)-1], "\r" + } + var mode string + if i := strings.IndexByte(rest, ' '); i >= 0 { + rest, mode = rest[:i], rest[i:] + } + i := strings.Index(rest, "..") + if i < 0 { + return header + } + return prefix + rest[i+2:] + ".." + rest[:i] + mode + suffix +} + // ReverseMultiFileDiff reverses a series of FileDiffs. func ReverseMultiFileDiff(fds []*FileDiff) ([]*FileDiff, error) { var reverse []*FileDiff diff --git a/diff/reverse_test.go b/diff/reverse_test.go index a656e54..9f33dd1 100644 --- a/diff/reverse_test.go +++ b/diff/reverse_test.go @@ -216,3 +216,92 @@ func TestReverseRoundTripOnTestdata(t *testing.T) { }) } } + +func TestReverseFileDiffExtendedHeaders(t *testing.T) { + tests := []struct { + name string + input []string + want []string + }{ + { + name: "new file", + input: []string{"diff --git a/f b/f", "new file mode 100644", "index 0000000..587be6b"}, + want: []string{"diff --git a/f b/f", "deleted file mode 100644", "index 587be6b..0000000"}, + }, + { + name: "deleted file", + input: []string{"diff --git a/f b/f", "deleted file mode 100644", "index 587be6b..0000000"}, + want: []string{"diff --git a/f b/f", "new file mode 100644", "index 0000000..587be6b"}, + }, + { + name: "rename", + input: []string{"diff --git a/old b/new", "similarity index 70%", "rename from old", "rename to new", "index 94954ab..8b14c4f 100644"}, + want: []string{"diff --git a/old b/new", "similarity index 70%", "rename from new", "rename to old", "index 8b14c4f..94954ab 100644"}, + }, + { + name: "copy", + input: []string{"diff --git a/old b/new", "similarity index 100%", "copy from old", "copy to new"}, + want: []string{"diff --git a/old b/new", "similarity index 100%", "copy from new", "copy to old"}, + }, + { + name: "mode change", + input: []string{"diff --git a/f b/f", "old mode 100644", "new mode 100755"}, + want: []string{"diff --git a/f b/f", "old mode 100755", "new mode 100644"}, + }, + { + name: "no extended headers", + input: nil, + want: nil, + }, + { + // Only git emits extended headers, so leave anything else alone. + name: "non-git header block", + input: []string{"diff --ruN a/f b/f", "old mode 0777", "new mode 0755"}, + want: []string{"diff --ruN a/f b/f", "old mode 0777", "new mode 0755"}, + }, + } + for _, test := range tests { + orig := append([]string(nil), test.input...) + fd := &FileDiff{OrigName: "a/f", NewName: "b/f", Extended: test.input} + reversed, err := ReverseFileDiff(fd) + if err != nil { + t.Errorf("%s: ReverseFileDiff: %s", test.name, err) + continue + } + if d := cmp.Diff(test.want, reversed.Extended); d != "" { + t.Errorf("%s: reversed extended headers differ (-want +got):\n%s", test.name, d) + } + if d := cmp.Diff(orig, fd.Extended); d != "" { + t.Errorf("%s: ReverseFileDiff mutated its input (-want +got):\n%s", test.name, d) + } + } +} + +func TestReverseFileDiffEmptyNewFile(t *testing.T) { + input := []byte("diff --git a/empty.txt b/empty.txt\nnew file mode 100644\nindex 0000000..e69de29\n") + fd, err := ParseFileDiff(input) + if err != nil { + t.Fatal(err) + } + reversed, err := ReverseFileDiff(fd) + if err != nil { + t.Fatal(err) + } + printed, err := PrintFileDiff(reversed) + if err != nil { + t.Fatal(err) + } + roundTrip, err := ParseFileDiff(printed) + if err != nil { + t.Fatal(err) + } + // The forward diff creates the file, so it parses as OrigName=/dev/null. + // Reversing it must delete the file, i.e. NewName=/dev/null. + if fd.OrigName != "/dev/null" { + t.Fatalf("forward diff: got OrigName=%q, want /dev/null", fd.OrigName) + } + if roundTrip.NewName != "/dev/null" || roundTrip.OrigName == "/dev/null" { + t.Errorf("reversed empty new-file diff: got OrigName=%q NewName=%q, want NewName=/dev/null\nprinted:\n%s", + roundTrip.OrigName, roundTrip.NewName, printed) + } +} From 027af28d6dbaaf905a33becd6fe3a67a8058db11 Mon Sep 17 00:00:00 2001 From: Keegan Carruthers-Smith Date: Thu, 10 Sep 2026 09:15:52 +0200 Subject: [PATCH 2/2] fix/reverse: reject copies that cannot be faithfully undone A Git copy records how to create the destination but not enough content to construct its inverse deletion. Returning an error avoids producing a patch that tries to overwrite the still-existing source, while the smaller index parser keeps mode and CRLF preservation explicit. Amp-Thread-ID: https://ampcode.com/threads/T-01a08a17-d51a-71a5-a028-1399ee2700e4 Co-authored-by: Amp --- diff/reverse.go | 42 +++++++++++++++++++++--------------------- diff/reverse_test.go | 28 +++++++++++++++++++++++----- 2 files changed, 44 insertions(+), 26 deletions(-) diff --git a/diff/reverse.go b/diff/reverse.go index 924b6b4..edbd32d 100644 --- a/diff/reverse.go +++ b/diff/reverse.go @@ -7,15 +7,21 @@ import ( "strings" ) -// ReverseFileDiff takes a diff.FileDiff, and returns the reverse operation. -// This is a FileDiff that undoes the edit of the original. +// ReverseFileDiff takes a diff.FileDiff and returns the reverse operation. +// This is a FileDiff that undoes the edit of the original. Git copy diffs +// cannot be reversed because they do not contain enough information to delete +// the copied file. func ReverseFileDiff(fd *FileDiff) (*FileDiff, error) { + extended, err := reverseExtendedHeaders(fd.Extended) + if err != nil { + return nil, err + } reverse := FileDiff{ OrigName: fd.NewName, OrigTime: fd.NewTime, NewName: fd.OrigName, NewTime: fd.OrigTime, - Extended: reverseExtendedHeaders(fd.Extended), + Extended: extended, } for _, hunk := range fd.Hunks { invHunk, err := reverseHunk(hunk) @@ -27,12 +33,11 @@ func ReverseFileDiff(fd *FileDiff) (*FileDiff, error) { return &reverse, nil } -// reverseExtendedHeaders reverses the direction encoded in git's extended -// header lines, matching what "git diff -R" emits. -func reverseExtendedHeaders(headers []string) []string { +// reverseExtendedHeaders reverses the direction encoded in git's extended headers. +func reverseExtendedHeaders(headers []string) ([]string, error) { // handleEmpty gates on the same prefix when it reads the direction back out. if len(headers) == 0 || !strings.HasPrefix(headers[0], "diff --git ") { - return headers + return headers, nil } reversed := make([]string, len(headers)) copy(reversed, headers) @@ -44,12 +49,13 @@ func reverseExtendedHeaders(headers []string) []string { reversed[i] = "new file mode " + header[len("deleted file mode "):] case strings.HasPrefix(header, "index "): reversed[i] = reverseIndexHeader(header) + case strings.HasPrefix(header, "copy from "), strings.HasPrefix(header, "copy to "): + return nil, errors.New("cannot reverse a git copy diff") } } swapHeaderValues(reversed, "old mode ", "new mode ") swapHeaderValues(reversed, "rename from ", "rename to ") - swapHeaderValues(reversed, "copy from ", "copy to ") - return reversed + return reversed, nil } // swapHeaderValues exchanges the values of the first "from" header and the @@ -74,20 +80,14 @@ func swapHeaderValues(headers []string, fromPrefix, toPrefix string) { // header, leaving the trailing mode (if any) alone. func reverseIndexHeader(header string) string { const prefix = "index " - rest := header[len(prefix):] - var suffix string - if strings.HasSuffix(rest, "\r") { - rest, suffix = rest[:len(rest)-1], "\r" - } - var mode string - if i := strings.IndexByte(rest, ' '); i >= 0 { - rest, mode = rest[:i], rest[i:] - } - i := strings.Index(rest, "..") - if i < 0 { + oldHash, newHash, ok := strings.Cut(header[len(prefix):], "..") + if !ok || strings.ContainsAny(oldHash, " \r") { return header } - return prefix + rest[i+2:] + ".." + rest[:i] + mode + suffix + if i := strings.IndexAny(newHash, " \r"); i >= 0 { + return prefix + newHash[:i] + ".." + oldHash + newHash[i:] + } + return prefix + newHash + ".." + oldHash } // ReverseMultiFileDiff reverses a series of FileDiffs. diff --git a/diff/reverse_test.go b/diff/reverse_test.go index 9f33dd1..3cc3962 100644 --- a/diff/reverse_test.go +++ b/diff/reverse_test.go @@ -169,6 +169,13 @@ func TestReverseRoundTripOnTestdata(t *testing.T) { } if fileDiffs, err := ParseMultiFileDiff(data); err == nil && len(fileDiffs) > 0 { + if name == "complicated_filenames.diff" { + if _, err := ReverseMultiFileDiff(fileDiffs); err == nil { + t.Fatal("reversing fixture with copy diffs succeeded") + } + return + } + reversed, err := ReverseMultiFileDiff(fileDiffs) if err != nil { t.Fatalf("first reverse: %s", err) @@ -238,16 +245,16 @@ func TestReverseFileDiffExtendedHeaders(t *testing.T) { input: []string{"diff --git a/old b/new", "similarity index 70%", "rename from old", "rename to new", "index 94954ab..8b14c4f 100644"}, want: []string{"diff --git a/old b/new", "similarity index 70%", "rename from new", "rename to old", "index 8b14c4f..94954ab 100644"}, }, - { - name: "copy", - input: []string{"diff --git a/old b/new", "similarity index 100%", "copy from old", "copy to new"}, - want: []string{"diff --git a/old b/new", "similarity index 100%", "copy from new", "copy to old"}, - }, { name: "mode change", input: []string{"diff --git a/f b/f", "old mode 100644", "new mode 100755"}, want: []string{"diff --git a/f b/f", "old mode 100755", "new mode 100644"}, }, + { + name: "CRLF index", + input: []string{"diff --git a/f b/f\r", "index 94954ab..8b14c4f 100644\r"}, + want: []string{"diff --git a/f b/f\r", "index 8b14c4f..94954ab 100644\r"}, + }, { name: "no extended headers", input: nil, @@ -277,6 +284,17 @@ func TestReverseFileDiffExtendedHeaders(t *testing.T) { } } +func TestReverseFileDiffRejectsCopy(t *testing.T) { + input := []byte("diff --git a/old b/new\nsimilarity index 100%\ncopy from old\ncopy to new\n") + fd, err := ParseFileDiff(input) + if err != nil { + t.Fatal(err) + } + if _, err := ReverseFileDiff(fd); err == nil { + t.Fatal("ReverseFileDiff succeeded for a copy diff") + } +} + func TestReverseFileDiffEmptyNewFile(t *testing.T) { input := []byte("diff --git a/empty.txt b/empty.txt\nnew file mode 100644\nindex 0000000..e69de29\n") fd, err := ParseFileDiff(input)