Skip to content

Commit 05194b3

Browse files
bugfix: binary-file-diff-panic
1 parent d5f760f commit 05194b3

2 files changed

Lines changed: 129 additions & 3 deletions

File tree

internal/git/diff.go

Lines changed: 29 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -112,11 +112,37 @@ type changesSinceContext struct {
112112
}
113113

114114
func diffToFilename(d *diff.FileDiff) string {
115-
if d.NewName == "/dev/null" {
116-
// For deleted files, NewName is "/dev/null", so use OrigName instead
115+
// For regular diffs, NewName/OrigName come from the "--- a/<path>" /
116+
// "+++ b/<path>" headers and carry an "a/"/"b/" prefix to strip. For
117+
// deleted files NewName is "/dev/null". For binary files git emits no
118+
// "---"/"+++" lines (only "Binary files a/<path> and b/<path> differ"),
119+
// leaving both fields empty — recover the path from the "diff --git"
120+
// extended header instead.
121+
if len(d.NewName) > 2 && d.NewName != "/dev/null" {
122+
return d.NewName[2:]
123+
}
124+
if len(d.OrigName) > 2 && d.OrigName != "/dev/null" {
117125
return d.OrigName[2:]
118126
}
119-
return d.NewName[2:]
127+
return filenameFromExtendedHeader(d.Extended)
128+
}
129+
130+
func filenameFromExtendedHeader(headers []string) string {
131+
for _, h := range headers {
132+
rest, ok := strings.CutPrefix(h, "diff --git ")
133+
if !ok {
134+
continue
135+
}
136+
// Quoted form: "a/<path>" "b/<path>" (paths needing escaping).
137+
if idx := strings.LastIndex(rest, ` "b/`); idx >= 0 {
138+
return strings.TrimSuffix(rest[idx+len(` "b/`):], `"`)
139+
}
140+
// Unquoted form: a/<path> b/<path>
141+
if idx := strings.LastIndex(rest, " b/"); idx >= 0 {
142+
return rest[idx+len(" b/"):]
143+
}
144+
}
145+
return ""
120146
}
121147

122148
// Parse the diff output to get the file names and hunks

internal/git/diff_test.go

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,52 @@ func TestNewDiff(t *testing.T) {
110110
"file2.go": 1,
111111
},
112112
},
113+
{
114+
name: "binary file change does not panic",
115+
context: DiffContext{
116+
Base: "main",
117+
Head: "feature",
118+
Dir: ".",
119+
},
120+
mockOutput: `diff --git a/file1.go b/file1.go
121+
index abc..def 100644
122+
--- a/file1.go
123+
+++ b/file1.go
124+
@@ -10,0 +11 @@ func Example() {
125+
+ fmt.Println("New line")
126+
diff --git a/assets/img/offline.png b/assets/img/offline.png
127+
index 1111111..2222222 100644
128+
Binary files a/assets/img/offline.png and b/assets/img/offline.png differ`,
129+
expectedErr: false,
130+
expectedFiles: 2,
131+
expectedHunks: map[string]int{
132+
"file1.go": 1,
133+
"assets/img/offline.png": 0,
134+
},
135+
},
136+
{
137+
name: "binary file in ignored directory is filtered",
138+
context: DiffContext{
139+
Base: "main",
140+
Head: "feature",
141+
Dir: ".",
142+
IgnoreDirs: []string{"assets/"},
143+
},
144+
mockOutput: `diff --git a/file1.go b/file1.go
145+
index abc..def 100644
146+
--- a/file1.go
147+
+++ b/file1.go
148+
@@ -10,0 +11 @@ func Example() {
149+
+ fmt.Println("New line")
150+
diff --git a/assets/img/offline.png b/assets/img/offline.png
151+
index 1111111..2222222 100644
152+
Binary files a/assets/img/offline.png and b/assets/img/offline.png differ`,
153+
expectedErr: false,
154+
expectedFiles: 1,
155+
expectedHunks: map[string]int{
156+
"file1.go": 1,
157+
},
158+
},
113159
{
114160
name: "ignore deleted files in ignored directories",
115161
context: DiffContext{
@@ -442,6 +488,60 @@ func TestToDiffFiles(t *testing.T) {
442488
},
443489
},
444490
},
491+
{
492+
name: "binary file (no --- / +++ headers, filename in extended)",
493+
fileDiffs: []*diff.FileDiff{
494+
{
495+
Extended: []string{
496+
"diff --git a/assets/img/offline.png b/assets/img/offline.png",
497+
"index abc123..def456 100644",
498+
"Binary files a/assets/img/offline.png and b/assets/img/offline.png differ",
499+
},
500+
},
501+
},
502+
expected: []codeowners.DiffFile{
503+
{
504+
FileName: "assets/img/offline.png",
505+
Hunks: []codeowners.HunkRange{},
506+
},
507+
},
508+
},
509+
{
510+
name: "binary file with quoted path",
511+
fileDiffs: []*diff.FileDiff{
512+
{
513+
Extended: []string{
514+
`diff --git "a/assets/some image.png" "b/assets/some image.png"`,
515+
"Binary files differ",
516+
},
517+
},
518+
},
519+
expected: []codeowners.DiffFile{
520+
{
521+
FileName: "assets/some image.png",
522+
Hunks: []codeowners.HunkRange{},
523+
},
524+
},
525+
},
526+
{
527+
name: "binary rename uses new path",
528+
fileDiffs: []*diff.FileDiff{
529+
{
530+
Extended: []string{
531+
"diff --git a/old/foo.png b/new/foo.png",
532+
"similarity index 100%",
533+
"rename from old/foo.png",
534+
"rename to new/foo.png",
535+
},
536+
},
537+
},
538+
expected: []codeowners.DiffFile{
539+
{
540+
FileName: "new/foo.png",
541+
Hunks: []codeowners.HunkRange{},
542+
},
543+
},
544+
},
445545
{
446546
name: "mixed: added, modified, and deleted files",
447547
fileDiffs: []*diff.FileDiff{

0 commit comments

Comments
 (0)