Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 31 additions & 3 deletions internal/git/diff.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
Expand Down
131 changes: 131 additions & 0 deletions internal/git/diff_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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{
Expand Down
Loading