Centralize Git extended-header parsing

diff/git_extended_headers.go

@@ -0,0 +1,94 @@
+package diff
+
+import "strings"
+
+const (
+	gitExtendedHeaderDiff            = "diff --git "
+	gitExtendedHeaderOldMode         = "old mode "
+	gitExtendedHeaderNewMode         = "new mode "
+	gitExtendedHeaderNewFileMode     = "new file mode "
+	gitExtendedHeaderDeletedFileMode = "deleted file mode "
+	gitExtendedHeaderRenameFrom      = "rename from "
+	gitExtendedHeaderRenameTo        = "rename to "
+	gitExtendedHeaderCopyFrom        = "copy from "
+	gitExtendedHeaderCopyTo          = "copy to "
+	gitExtendedHeaderIndex           = "index "
+	gitExtendedHeaderBinaryFiles     = "Binary files "
+	gitExtendedHeaderBinaryPatch     = "GIT binary patch"
+)
+
+var knownGitExtendedHeaderKinds = [...]string{
+	gitExtendedHeaderDiff,
+	gitExtendedHeaderOldMode,
+	gitExtendedHeaderNewMode,
+	gitExtendedHeaderNewFileMode,
+	gitExtendedHeaderDeletedFileMode,
+	gitExtendedHeaderRenameFrom,
+	gitExtendedHeaderRenameTo,
+	gitExtendedHeaderCopyFrom,
+	gitExtendedHeaderCopyTo,
+	gitExtendedHeaderIndex,
+	gitExtendedHeaderBinaryFiles,
+	gitExtendedHeaderBinaryPatch,
+}
+
+type gitExtendedHeader struct {
+	raw  string
+	kind string
+}
+
+func (h gitExtendedHeader) value() string {
+	return h.raw[len(h.kind):]
+}
+
+type gitExtendedHeaders []gitExtendedHeader
+
+func parseGitExtendedHeaders(raw []string) (gitExtendedHeaders, bool) {
+	if len(raw) == 0 || !strings.HasPrefix(raw[0], gitExtendedHeaderDiff) {
+		return nil, false
+	}
+
+	headers := make(gitExtendedHeaders, len(raw))
+	for i, line := range raw {
+		headers[i].raw = line
+		for _, kind := range knownGitExtendedHeaderKinds {
+			if strings.HasPrefix(line, kind) {
+				headers[i].kind = kind
+				break
+			}
+		}
+	}
+	return headers, true
+}
+
+type gitExtendedHeaderPair struct {
+	from string
+	to   string
+}
+
+var (
+	gitModeHeaderPair   = gitExtendedHeaderPair{gitExtendedHeaderOldMode, gitExtendedHeaderNewMode}
+	gitRenameHeaderPair = gitExtendedHeaderPair{gitExtendedHeaderRenameFrom, gitExtendedHeaderRenameTo}
+	gitCopyHeaderPair   = gitExtendedHeaderPair{gitExtendedHeaderCopyFrom, gitExtendedHeaderCopyTo}
+)
+
+func (h gitExtendedHeaders) hasKind(index int, kind string) bool {
+	return index >= 0 && index < len(h) && h[index].kind == kind
+}
+
+func (h gitExtendedHeaders) hasPairAt(index int, pair gitExtendedHeaderPair) bool {
+	return h.hasKind(index, pair.from) && h.hasKind(index+1, pair.to)
+}
+
+func (h gitExtendedHeaders) pairIndices(pair gitExtendedHeaderPair) (from, to int, ok bool) {
+	from, to = -1, -1
+	for i, header := range h {
+		if from < 0 && header.kind == pair.from {
+			from = i
+		}
+		if to < 0 && header.kind == pair.to {
+			to = i
+		}
+	}
+	return from, to, from >= 0 && to >= 0
+}

diff/git_extended_headers_test.go

@@ -0,0 +1,70 @@
+package diff
+
+import (
+	"bytes"
+	"errors"
+	"reflect"
+	"testing"
+)
+
+func TestParseFileDiffPreservesCRLFGitExtendedHeaders(t *testing.T) {
+	input := []byte("diff --git a/old name b/new name\r\nsimilarity index 100%\r\nrename from old name\r\nrename to new name\r\n")
+	wantExtended := []string{
+		"diff --git a/old name b/new name\r",
+		"similarity index 100%\r",
+		"rename from old name\r",
+		"rename to new name\r",
+	}
+
+	fd, err := ParseFileDiffOptions(input, ParseOptions{KeepCR: true})
+	if err != nil {
+		t.Fatal(err)
+	}
+	// Keep the existing edge behavior: with KeepCR, reconstruction
+	// finds the first ambiguous path but not the second.
+	if fd.OrigName != "a/old name" || fd.NewName != "" {
+		t.Errorf("got names %q -> %q, want %q -> empty", fd.OrigName, fd.NewName, "a/old name")
+	}
+	if !reflect.DeepEqual(fd.Extended, wantExtended) {
+		t.Errorf("extended headers changed:\nwant: %q\n got: %q", wantExtended, fd.Extended)
+	}
+}
+
+func TestHandleEmptyRejectsMalformedGitExtendedHeaders(t *testing.T) {
+	tests := []struct {
+		name     string
+		extended []string
+	}{
+		{name: "empty"},
+		{name: "non-git", extended: []string{"diff --ruN a/f b/f", "old mode 100644", "new mode 100755"}},
+		{name: "reversed mode pair", extended: []string{"diff --git a/f b/f", "new mode 100755", "old mode 100644"}},
+		{name: "copy prefixes missing spaces", extended: []string{"diff --git a/f b/g", "similarity index 100%", "copy from", "copy to"}},
+		{name: "unknown extra header", extended: []string{"diff --git a/f b/f", "old mode 100644", "new mode 100755", "x-header value"}},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			fd := &FileDiff{Extended: test.extended}
+			if handleEmpty(fd) {
+				t.Fatal("handleEmpty accepted malformed headers")
+			}
+			if fd.OrigName != "" || fd.NewName != "" {
+				t.Errorf("handleEmpty changed names to %q -> %q", fd.OrigName, fd.NewName)
+			}
+		})
+	}
+}
+
+func TestMalformedGitExtendedHeadersStillReturnOriginalParseError(t *testing.T) {
+	input := []byte("diff --git a/f b/f\nold mode 100644\nunknown header\n")
+	r := NewFileDiffReader(bytes.NewReader(input))
+	fd, err := r.ReadAllHeaders()
+	var parseErr *ParseError
+	if !errors.As(err, &parseErr) || parseErr.Err != ErrExtendedHeadersEOF {
+		t.Fatalf("got error %v, want ErrExtendedHeadersEOF", err)
+	}
+	wantExtended := []string{"diff --git a/f b/f", "old mode 100644", "unknown header"}
+	if !reflect.DeepEqual(fd.Extended, wantExtended) {
+		t.Errorf("extended headers changed:\nwant: %q\n got: %q", wantExtended, fd.Extended)
+	}
+}

diff/parse.go

@@ -373,7 +373,7 @@ func (r *FileDiffReader) ReadExtendedHeaders() ([]string, error) {
 			r.fileHeaderLine = nil
 		}
 
-		if bytes.HasPrefix(line, []byte("diff --git ")) {
+		if bytes.HasPrefix(line, []byte(gitExtendedHeaderDiff)) {
 			if firstLine {
 				firstLine = false
 			} else {
@@ -508,43 +508,36 @@ func parseDiffGitArgs(diffArgs string) (string, string, bool) {
 // that follow. It updates fd fields from the parsed extended headers.
 func handleEmpty(fd *FileDiff) (wasEmpty bool) {
 	lineCount := len(fd.Extended)
-	if lineCount > 0 && !strings.HasPrefix(fd.Extended[0], "diff --git ") {
+	headers, ok := parseGitExtendedHeaders(fd.Extended)
+	if !ok {
 		return false
 	}
 
-	lineHasPrefix := func(idx int, prefix string) bool {
-		return strings.HasPrefix(fd.Extended[idx], prefix)
-	}
-
-	linesHavePrefixes := func(idx1 int, prefix1 string, idx2 int, prefix2 string) bool {
-		return lineHasPrefix(idx1, prefix1) && lineHasPrefix(idx2, prefix2)
-	}
-
-	isCopy := (lineCount == 4 && linesHavePrefixes(2, "copy from ", 3, "copy to ")) ||
-		(lineCount == 6 && linesHavePrefixes(2, "copy from ", 3, "copy to ") && lineHasPrefix(5, "Binary files ")) ||
-		(lineCount == 6 && linesHavePrefixes(1, "old mode ", 2, "new mode ") && linesHavePrefixes(4, "copy from ", 5, "copy to "))
+	isCopy := (lineCount == 4 && headers.hasPairAt(2, gitCopyHeaderPair)) ||
+		(lineCount == 6 && headers.hasPairAt(2, gitCopyHeaderPair) && headers.hasKind(5, gitExtendedHeaderBinaryFiles)) ||
+		(lineCount == 6 && headers.hasPairAt(1, gitModeHeaderPair) && headers.hasPairAt(4, gitCopyHeaderPair))
 
-	isRename := (lineCount == 4 && linesHavePrefixes(2, "rename from ", 3, "rename to ")) ||
-		(lineCount == 5 && linesHavePrefixes(2, "rename from ", 3, "rename to ") && lineHasPrefix(4, "Binary files ")) ||
-		(lineCount == 6 && linesHavePrefixes(2, "rename from ", 3, "rename to ") && lineHasPrefix(5, "Binary files ")) ||
-		(lineCount == 6 && linesHavePrefixes(1, "old mode ", 2, "new mode ") && linesHavePrefixes(4, "rename from ", 5, "rename to "))
+	isRename := (lineCount == 4 && headers.hasPairAt(2, gitRenameHeaderPair)) ||
+		(lineCount == 5 && headers.hasPairAt(2, gitRenameHeaderPair) && headers.hasKind(4, gitExtendedHeaderBinaryFiles)) ||
+		(lineCount == 6 && headers.hasPairAt(2, gitRenameHeaderPair) && headers.hasKind(5, gitExtendedHeaderBinaryFiles)) ||
+		(lineCount == 6 && headers.hasPairAt(1, gitModeHeaderPair) && headers.hasPairAt(4, gitRenameHeaderPair))
 
-	isDeletedFile := (lineCount == 3 || lineCount == 4 && lineHasPrefix(3, "Binary files ") || lineCount > 4 && lineHasPrefix(3, "GIT binary patch")) &&
-		lineHasPrefix(1, "deleted file mode ")
+	isDeletedFile := (lineCount == 3 || lineCount == 4 && headers.hasKind(3, gitExtendedHeaderBinaryFiles) || lineCount > 4 && headers.hasKind(3, gitExtendedHeaderBinaryPatch)) &&
+		headers.hasKind(1, gitExtendedHeaderDeletedFileMode)
 
-	isNewFile := (lineCount == 3 || lineCount == 4 && lineHasPrefix(3, "Binary files ") || lineCount > 4 && lineHasPrefix(3, "GIT binary patch")) &&
-		lineHasPrefix(1, "new file mode ")
+	isNewFile := (lineCount == 3 || lineCount == 4 && headers.hasKind(3, gitExtendedHeaderBinaryFiles) || lineCount > 4 && headers.hasKind(3, gitExtendedHeaderBinaryPatch)) &&
+		headers.hasKind(1, gitExtendedHeaderNewFileMode)
 
-	isModeChange := lineCount == 3 && linesHavePrefixes(1, "old mode ", 2, "new mode ")
+	isModeChange := lineCount == 3 && headers.hasPairAt(1, gitModeHeaderPair)
 
-	isBinaryPatch := lineCount == 3 && lineHasPrefix(2, "Binary files ") || lineCount > 3 && lineHasPrefix(2, "GIT binary patch")
+	isBinaryPatch := lineCount == 3 && headers.hasKind(2, gitExtendedHeaderBinaryFiles) || lineCount > 3 && headers.hasKind(2, gitExtendedHeaderBinaryPatch)
 
 	if !isModeChange && !isCopy && !isRename && !isBinaryPatch && !isNewFile && !isDeletedFile {
 		return false
 	}
 
 	var success bool
-	fd.OrigName, fd.NewName, success = parseDiffGitArgs(fd.Extended[0][len("diff --git "):])
+	fd.OrigName, fd.NewName, success = parseDiffGitArgs(headers[0].value())
 	if isNewFile {
 		fd.OrigName = "/dev/null"
 	}
@@ -555,13 +548,10 @@ func handleEmpty(fd *FileDiff) (wasEmpty bool) {
 
 	// For ambiguous 'diff --git' lines, try to reconstruct filenames using extended headers.
 	if success && (isCopy || isRename) && fd.OrigName == "" && fd.NewName == "" {
-		diffArgs := fd.Extended[0][len("diff --git "):]
+		diffArgs := headers[0].value()
 
-		tryReconstruct := func(header string, prefix string, whichFile int, result *string) {
-			if !strings.HasPrefix(header, prefix) {
-				return
-			}
-			rawFilename := header[len(prefix):]
+		tryReconstruct := func(header gitExtendedHeader, whichFile int, result *string) {
+			rawFilename := header.value()
 			rawFilename = strings.TrimSuffix(rawFilename, "\r")
 
 			// extract the filename prefix (e.g. "a/") from the 'diff --git' line.
@@ -578,11 +568,13 @@ func handleEmpty(fd *FileDiff) (wasEmpty bool) {
 			*result = diffArgs[prefixLetterIndex:prefixLetterIndex+2] + rawFilename
 		}
 
-		for _, header := range fd.Extended {
-			tryReconstruct(header, "copy from ", 1, &fd.OrigName)
-			tryReconstruct(header, "copy to ", 2, &fd.NewName)
-			tryReconstruct(header, "rename from ", 1, &fd.OrigName)
-			tryReconstruct(header, "rename to ", 2, &fd.NewName)
+		for _, header := range headers {
+			switch header.kind {
+			case gitExtendedHeaderCopyFrom, gitExtendedHeaderRenameFrom:
+				tryReconstruct(header, 1, &fd.OrigName)
+			case gitExtendedHeaderCopyTo, gitExtendedHeaderRenameTo:
+				tryReconstruct(header, 2, &fd.NewName)
+			}
 		}
 	}
 	return success

diff/reverse.go

@@ -38,27 +38,27 @@ func ReverseFileDiff(fd *FileDiff) (*FileDiff, error) {
 
 // reverseExtendedHeaders reverses the direction encoded in git's extended headers.
 func reverseExtendedHeaders(headers []string, origName, newName 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 ") {
+	parsed, ok := parseGitExtendedHeaders(headers)
+	if !ok {
 		return headers, nil
 	}
 	reversed := make([]string, len(headers))
 	copy(reversed, headers)
 	reversed[0] = reverseDiffGitHeader(reversed[0], origName, newName)
-	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 "):
+	for i, header := range parsed {
+		switch header.kind {
+		case gitExtendedHeaderNewFileMode:
+			reversed[i] = gitExtendedHeaderDeletedFileMode + header.value()
+		case gitExtendedHeaderDeletedFileMode:
+			reversed[i] = gitExtendedHeaderNewFileMode + header.value()
+		case gitExtendedHeaderIndex:
 			reversed[i] = reverseIndexHeader(header)
-		case strings.HasPrefix(header, "copy from "), strings.HasPrefix(header, "copy to "):
+		case gitExtendedHeaderCopyFrom, gitExtendedHeaderCopyTo:
 			return nil, ErrCannotReverseCopy
 		}
 	}
-	swapHeaderValues(reversed, "old mode ", "new mode ")
-	swapHeaderValues(reversed, "rename from ", "rename to ")
+	swapHeaderValues(reversed, parsed, gitModeHeaderPair)
+	swapHeaderValues(reversed, parsed, gitRenameHeaderPair)
 	return reversed, nil
 }
 
@@ -67,7 +67,7 @@ func reverseExtendedHeaders(headers []string, origName, newName string) ([]strin
 // ambiguous input; names recovered from other headers can disambiguate Git's
 // unquoted paths containing spaces.
 func reverseDiffGitHeader(header, origName, newName string) string {
-	const prefix = "diff --git "
+	const prefix = gitExtendedHeaderDiff
 	args := header[len(prefix):]
 	lineEnding := ""
 	if strings.HasSuffix(args, "\r") {
@@ -122,30 +122,23 @@ func splitDiffGitArgs(args, first, second string) (string, string, bool) {
 
 // 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 {
+func swapHeaderValues(headers []string, parsed gitExtendedHeaders, pair gitExtendedHeaderPair) {
+	from, to, ok := parsed.pairIndices(pair)
+	if !ok {
 		return
 	}
-	headers[from], headers[to] = fromPrefix+headers[to][len(toPrefix):], toPrefix+headers[from][len(fromPrefix):]
+	headers[from] = pair.from + parsed[to].value()
+	headers[to] = pair.to + parsed[from].value()
 }
 
 // reverseIndexHeader swaps the two blob hashes in an "index <old>..<new>[ <mode>]"
 // header, leaving the trailing mode (if any) alone.
-func reverseIndexHeader(header string) string {
-	const prefix = "index "
-	oldHash, newHash, ok := strings.Cut(header[len(prefix):], "..")
+func reverseIndexHeader(header gitExtendedHeader) string {
+	oldHash, newHash, ok := strings.Cut(header.value(), "..")
 	if !ok || strings.ContainsAny(oldHash, " \r") {
-		return header
+		return header.raw
 	}
+	prefix := gitExtendedHeaderIndex
 	if i := strings.IndexAny(newHash, " \r"); i >= 0 {
 		return prefix + newHash[:i] + ".." + oldHash + newHash[i:]
 	}

diff/reverse_test.go

@@ -255,6 +255,16 @@ func TestReverseFileDiffExtendedHeaders(t *testing.T) {
 			input: []string{"diff --git a/f b/f\r", "index 94954ab..8b14c4f 100644\r"},
 			want:  []string{"diff --git b/f a/f\r", "index 8b14c4f..94954ab 100644\r"},
 		},
+		{
+			name:  "CRLF mode and rename",
+			input: []string{"diff --git a/old b/new\r", "old mode 100644\r", "new mode 100755\r", "rename from old\r", "rename to new\r"},
+			want:  []string{"diff --git b/new a/old\r", "old mode 100755\r", "new mode 100644\r", "rename from new\r", "rename to old\r"},
+		},
+		{
+			name:  "unknown and malformed headers",
+			input: []string{"diff --git a/f b/f", "x-header value", "old mode 100644", "copy from", "index missing-separator"},
+			want:  []string{"diff --git b/f a/f", "x-header value", "old mode 100644", "copy from", "index missing-separator"},
+		},
 		{
 			name:  "no extended headers",
 			input: nil,
@@ -343,13 +353,15 @@ func TestReverseFileDiffGitHeader(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)
+	tests := [][]string{
+		{"diff --git a/old b/new", "similarity index 100%", "copy from old", "copy to new"},
+		{"diff --git a/old b/new", "copy from old"},
 	}
-	if _, err := ReverseFileDiff(fd); err != ErrCannotReverseCopy {
-		t.Fatalf("ReverseFileDiff error = %v, want ErrCannotReverseCopy", err)
+	for _, extended := range tests {
+		fd := &FileDiff{Extended: extended}
+		if _, err := ReverseFileDiff(fd); err != ErrCannotReverseCopy {
+			t.Errorf("ReverseFileDiff error = %v, want ErrCannotReverseCopy for headers %q", err, extended)
+		}
 	}
 }