bugfix: binary file panic

Author: asyncawaitpromise

internal/git/diff.go

@@ -112,11 +112,37 @@ type changesSinceContext struct {
 }
 
 func diffToFilename(d *diff.FileDiff) string {
-	if d.NewName == "/dev/null" {
-		// For deleted files, NewName is "/dev/null", so use OrigName instead
+	// For regular diffs, NewName/OrigName come from the "--- a/<path>" /
+	// "+++ b/<path>" headers and carry an "a/"/"b/" prefix to strip. For
+	// deleted files NewName is "/dev/null". For binary files git emits no
+	// "---"/"+++" lines (only "Binary files a/<path> and b/<path> differ"),
+	// leaving both fields empty — recover the path from the "diff --git"
+	// extended header instead.
+	if len(d.NewName) > 2 && d.NewName != "/dev/null" {
+		return d.NewName[2:]
+	}
+	if len(d.OrigName) > 2 && d.OrigName != "/dev/null" {
 		return d.OrigName[2:]
 	}
-	return d.NewName[2:]
+	return filenameFromExtendedHeader(d.Extended)
+}
+
+func filenameFromExtendedHeader(headers []string) string {
+	for _, h := range headers {
+		rest, ok := strings.CutPrefix(h, "diff --git ")
+		if !ok {
+			continue
+		}
+		// Quoted form: "a/<path>" "b/<path>" (paths needing escaping).
+		if idx := strings.LastIndex(rest, ` "b/`); idx >= 0 {
+			return strings.TrimSuffix(rest[idx+len(` "b/`):], `"`)
+		}
+		// Unquoted form: a/<path> b/<path>
+		if idx := strings.LastIndex(rest, " b/"); idx >= 0 {
+			return rest[idx+len(" b/"):]
+		}
+	}
+	return ""
 }
 
 // Parse the diff output to get the file names and hunks
@@ -172,6 +198,8 @@ func changesSince(context changesSinceContext) ([]codeowners.DiffFile, error) {
 				newDiffFile.Hunks = append(newDiffFile.Hunks, newHunkRange)
 			}
 		}
+		// Binary files have no hunks; staleness is intentionally not tracked
+		// for them (there is no hunk content to hash against the older diff).
 		if len(newDiffFile.Hunks) > 0 {
 			diffFiles = append(diffFiles, newDiffFile)
 		}

internal/git/diff_test.go

@@ -110,6 +110,82 @@ func TestNewDiff(t *testing.T) {
 				"file2.go": 1,
 			},
 		},
+		{
+			name: "binary file change does not panic",
+			context: DiffContext{
+				Base: "main",
+				Head: "feature",
+				Dir:  ".",
+			},
+			mockOutput: `diff --git a/file1.go b/file1.go
+index abc..def 100644
+--- a/file1.go
++++ b/file1.go
+@@ -10,0 +11 @@ func Example() {
++       fmt.Println("New line")
+diff --git a/assets/img/offline.png b/assets/img/offline.png
+index 1111111..2222222 100644
+Binary files a/assets/img/offline.png and b/assets/img/offline.png differ`,
+			expectedErr:   false,
+			expectedFiles: 2,
+			expectedHunks: map[string]int{
+				"file1.go":                1,
+				"assets/img/offline.png": 0,
+			},
+		},
+		{
+			// Regression for PE-887: a "mode change + binary patch" entry
+			// is parsed by go-diff into a FileDiff with empty OrigName/NewName
+			// (its handleEmpty logic does not cover this 5-extended-header
+			// shape). The pre-fix code did NewName[2:] and panicked.
+			name: "mode change + binary patch (PE-887)",
+			context: DiffContext{
+				Base: "main",
+				Head: "feature",
+				Dir:  ".",
+			},
+			mockOutput: `diff --git a/assets/img.png b/assets/img.png
+old mode 100755
+new mode 100644
+index 8ed9b15..0bfca45
+Binary files a/assets/img.png and b/assets/img.png differ
+diff --git a/notes.txt b/notes.txt
+index abc..def 100644
+--- a/notes.txt
++++ b/notes.txt
+@@ -1,0 +2 @@ trailing entry forces parser to flush the mode-change+binary above
++ok
+`,
+			expectedErr:   false,
+			expectedFiles: 2,
+			expectedHunks: map[string]int{
+				"assets/img.png": 0,
+				"notes.txt":      1,
+			},
+		},
+		{
+			name: "binary file in ignored directory is filtered",
+			context: DiffContext{
+				Base:       "main",
+				Head:       "feature",
+				Dir:        ".",
+				IgnoreDirs: []string{"assets/"},
+			},
+			mockOutput: `diff --git a/file1.go b/file1.go
+index abc..def 100644
+--- a/file1.go
++++ b/file1.go
+@@ -10,0 +11 @@ func Example() {
++       fmt.Println("New line")
+diff --git a/assets/img/offline.png b/assets/img/offline.png
+index 1111111..2222222 100644
+Binary files a/assets/img/offline.png and b/assets/img/offline.png differ`,
+			expectedErr:   false,
+			expectedFiles: 1,
+			expectedHunks: map[string]int{
+				"file1.go": 1,
+			},
+		},
 		{
 			name: "ignore deleted files in ignored directories",
 			context: DiffContext{
@@ -442,6 +518,60 @@ func TestToDiffFiles(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "binary file (no --- / +++ headers, filename in extended)",
+			fileDiffs: []*diff.FileDiff{
+				{
+					Extended: []string{
+						"diff --git a/assets/img/offline.png b/assets/img/offline.png",
+						"index abc123..def456 100644",
+						"Binary files a/assets/img/offline.png and b/assets/img/offline.png differ",
+					},
+				},
+			},
+			expected: []codeowners.DiffFile{
+				{
+					FileName: "assets/img/offline.png",
+					Hunks:    []codeowners.HunkRange{},
+				},
+			},
+		},
+		{
+			name: "binary file with quoted path",
+			fileDiffs: []*diff.FileDiff{
+				{
+					Extended: []string{
+						`diff --git "a/assets/some image.png" "b/assets/some image.png"`,
+						"Binary files differ",
+					},
+				},
+			},
+			expected: []codeowners.DiffFile{
+				{
+					FileName: "assets/some image.png",
+					Hunks:    []codeowners.HunkRange{},
+				},
+			},
+		},
+		{
+			name: "binary rename uses new path",
+			fileDiffs: []*diff.FileDiff{
+				{
+					Extended: []string{
+						"diff --git a/old/foo.png b/new/foo.png",
+						"similarity index 100%",
+						"rename from old/foo.png",
+						"rename to new/foo.png",
+					},
+				},
+			},
+			expected: []codeowners.DiffFile{
+				{
+					FileName: "new/foo.png",
+					Hunks:    []codeowners.HunkRange{},
+				},
+			},
+		},
 		{
 			name: "mixed: added, modified, and deleted files",
 			fileDiffs: []*diff.FileDiff{