From 202555a661cd9504e1dfe43b7d410b837fcd2352 Mon Sep 17 00:00:00 2001 From: Ezekiel Lopez Date: Tue, 19 May 2026 22:32:21 -0700 Subject: [PATCH] bugfix: binary file panic --- internal/git/diff.go | 34 +++++++++- internal/git/diff_test.go | 131 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 162 insertions(+), 3 deletions(-) diff --git a/internal/git/diff.go b/internal/git/diff.go index efc2488..95c74c0 100644 --- a/internal/git/diff.go +++ b/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/" / + // "+++ b/" 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/ and b/ 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/" "b/" (paths needing escaping). + if idx := strings.LastIndex(rest, ` "b/`); idx >= 0 { + return strings.TrimSuffix(rest[idx+len(` "b/`):], `"`) + } + // Unquoted form: a/ b/ + 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) } diff --git a/internal/git/diff_test.go b/internal/git/diff_test.go index ca5df80..59a2244 100644 --- a/internal/git/diff_test.go +++ b/internal/git/diff_test.go @@ -110,6 +110,83 @@ 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 the production panic: 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", + 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 +519,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{