Skip to content

Handle rename-and-mode-change in empty file diffs & clean up code #58

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 7, 2021
Merged

Handle rename-and-mode-change in empty file diffs & clean up code #58

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
640c65b
Extend sample tests, including failing test
mrnugget Jan 5, 2021
d9b0834
Handle rename-and-mode-change case (verbosely for now)
mrnugget Jan 5, 2021
3a166d4
Simplify handling of empty file diffs
mrnugget Jan 5, 2021
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
87 changes: 61 additions & 26 deletions diff/diff_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,20 @@ func TestParseFileDiffHeaders(t *testing.T) {
},
},
},
{
filename: "sample_file_extended_empty_mode_change.diff",
wantDiff: &FileDiff{
OrigName: "a/docs/index.md",
OrigTime: nil,
NewName: "b/docs/index.md",
NewTime: nil,
Extended: []string{
"diff --git a/docs/index.md b/docs/index.md",
"old mode 100644",
"new mode 100755",
},
},
},
{
filename: "sample_file_extended_empty_new_binary.diff",
wantDiff: &FileDiff{
Expand Down Expand Up @@ -209,6 +223,23 @@ func TestParseFileDiffHeaders(t *testing.T) {
},
},
},
{
filename: "sample_file_extended_empty_rename_and_mode_change.diff",
wantDiff: &FileDiff{
OrigName: "a/textfile.txt",
OrigTime: nil,
NewName: "b/textfile2.txt",
NewTime: nil,
Extended: []string{
"diff --git a/textfile.txt b/textfile2.txt",
"old mode 100644",
"new mode 100755",
"similarity index 100%",
"rename from textfile.txt",
"rename to textfile2.txt",
},
},
},
{
filename: "quoted_filename.diff",
wantDiff: &FileDiff{
Expand Down Expand Up @@ -241,19 +272,21 @@ func TestParseFileDiffHeaders(t *testing.T) {
},
}
for _, test := range tests {
diffData, err := ioutil.ReadFile(filepath.Join("testdata", test.filename))
if err != nil {
t.Fatal(err)
}
diff, err := ParseFileDiff(diffData)
if err != nil {
t.Fatalf("%s: got ParseFileDiff error %v", test.filename, err)
}
t.Run(test.filename, func(t *testing.T) {
diffData, err := ioutil.ReadFile(filepath.Join("testdata", test.filename))
if err != nil {
t.Fatal(err)
}
diff, err := ParseFileDiff(diffData)
if err != nil {
t.Fatalf("%s: got ParseFileDiff error %v", test.filename, err)
}

diff.Hunks = nil
if got, want := diff, test.wantDiff; !cmp.Equal(got, want) {
t.Errorf("%s:\n\ngot - want:\n%s", test.filename, cmp.Diff(want, got))
}
diff.Hunks = nil
if got, want := diff, test.wantDiff; !cmp.Equal(got, want) {
t.Errorf("%s:\n\ngot - want:\n%s", test.filename, cmp.Diff(want, got))
}
})
}
}

Expand Down Expand Up @@ -672,21 +705,23 @@ func TestParseMultiFileDiffHeaders(t *testing.T) {
},
}
for _, test := range tests {
diffData, err := ioutil.ReadFile(filepath.Join("testdata", test.filename))
if err != nil {
t.Fatal(err)
}
diffs, err := ParseMultiFileDiff(diffData)
if err != nil {
t.Fatalf("%s: got ParseMultiFileDiff error %v", test.filename, err)
}
t.Run(test.filename, func(t *testing.T) {
diffData, err := ioutil.ReadFile(filepath.Join("testdata", test.filename))
if err != nil {
t.Fatal(err)
}
diffs, err := ParseMultiFileDiff(diffData)
if err != nil {
t.Fatalf("%s: got ParseMultiFileDiff error %v", test.filename, err)
}

for i := range diffs {
diffs[i].Hunks = nil // This test focuses on things other than hunks, so don't compare them.
}
if got, want := diffs, test.wantDiffs; !cmp.Equal(got, want) {
t.Errorf("%s:\n\ngot - want:\n%s", test.filename, cmp.Diff(want, got))
}
for i := range diffs {
diffs[i].Hunks = nil // This test focuses on things other than hunks, so don't compare them.
}
if got, want := diffs, test.wantDiffs; !cmp.Equal(got, want) {
t.Errorf("%s:\n\ngot - want:\n%s", test.filename, cmp.Diff(want, got))
}
})
}
}

Expand Down
101 changes: 42 additions & 59 deletions diff/parse.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,75 +357,58 @@ func (r *FileDiffReader) ReadExtendedHeaders() ([]string, error) {
// handleEmpty detects when FileDiff was an empty diff and will not have any hunks
// that follow. It updates fd fields from the parsed extended headers.
func handleEmpty(fd *FileDiff) (wasEmpty bool) {
var err error
lineCount := len(fd.Extended)
if lineCount > 0 && !strings.HasPrefix(fd.Extended[0], "diff --git ") {
return false
}
switch {
case (lineCount == 3 || lineCount == 4 && strings.HasPrefix(fd.Extended[3], "Binary files ") || lineCount > 4 && strings.HasPrefix(fd.Extended[3], "GIT binary patch")) &&
strings.HasPrefix(fd.Extended[1], "old mode ") && strings.HasPrefix(fd.Extended[2], "new mode "):

names := strings.SplitN(fd.Extended[0][len("diff --git "):], " ", 2)
fd.OrigName, err = strconv.Unquote(names[0])
if err != nil {
fd.OrigName = names[0]
}
fd.NewName, err = strconv.Unquote(names[1])
if err != nil {
fd.NewName = names[1]
}
return true
case (lineCount == 3 || lineCount == 4 && strings.HasPrefix(fd.Extended[3], "Binary files ") || lineCount > 4 && strings.HasPrefix(fd.Extended[3], "GIT binary patch")) &&
strings.HasPrefix(fd.Extended[1], "new file mode "):
lineHasPrefix := func(idx int, prefix string) bool {
return strings.HasPrefix(fd.Extended[idx], prefix)
}

names := strings.SplitN(fd.Extended[0][len("diff --git "):], " ", 2)
fd.OrigName = "/dev/null"
fd.NewName, err = strconv.Unquote(names[1])
if err != nil {
fd.NewName = names[1]
}
return true
case (lineCount == 3 || lineCount == 4 && strings.HasPrefix(fd.Extended[3], "Binary files ") || lineCount > 4 && strings.HasPrefix(fd.Extended[3], "GIT binary patch")) &&
strings.HasPrefix(fd.Extended[1], "deleted file mode "):
linesHavePrefixes := func(idx1 int, prefix1 string, idx2 int, prefix2 string) bool {
return lineHasPrefix(idx1, prefix1) && lineHasPrefix(idx2, prefix2)
}

names := strings.SplitN(fd.Extended[0][len("diff --git "):], " ", 2)
fd.OrigName, err = strconv.Unquote(names[0])
if err != nil {
fd.OrigName = names[0]
}
fd.NewName = "/dev/null"
return true
case lineCount == 4 && strings.HasPrefix(fd.Extended[2], "rename from ") && strings.HasPrefix(fd.Extended[3], "rename to "):
names := strings.SplitN(fd.Extended[0][len("diff --git "):], " ", 2)
fd.OrigName, err = strconv.Unquote(names[0])
if err != nil {
fd.OrigName = names[0]
}
fd.NewName, err = strconv.Unquote(names[1])
if err != nil {
fd.NewName = names[1]
}
return true
case lineCount == 6 && strings.HasPrefix(fd.Extended[5], "Binary files ") && strings.HasPrefix(fd.Extended[2], "rename from ") && strings.HasPrefix(fd.Extended[3], "rename to "):
names := strings.SplitN(fd.Extended[0][len("diff --git "):], " ", 2)
isRename := (lineCount == 4 && linesHavePrefixes(2, "rename from ", 3, "rename to ")) ||
(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 "))

isDeletedFile := (lineCount == 3 || lineCount == 4 && lineHasPrefix(3, "Binary files ") || lineCount > 4 && lineHasPrefix(3, "GIT binary patch")) &&
lineHasPrefix(1, "deleted file mode ")

isNewFile := (lineCount == 3 || lineCount == 4 && lineHasPrefix(3, "Binary files ") || lineCount > 4 && lineHasPrefix(3, "GIT binary patch")) &&
lineHasPrefix(1, "new file mode ")

isModeChange := lineCount == 3 && linesHavePrefixes(1, "old mode ", 2, "new mode ")

isBinaryPatch := lineCount == 3 && lineHasPrefix(2, "Binary files ") || lineCount > 3 && lineHasPrefix(2, "GIT binary patch")

if !isModeChange && !isRename && !isBinaryPatch && !isNewFile && !isDeletedFile {
return false
}

names := strings.SplitN(fd.Extended[0][len("diff --git "):], " ", 2)

var err error
fd.OrigName, err = strconv.Unquote(names[0])
if err != nil {
fd.OrigName = names[0]
}
fd.NewName, err = strconv.Unquote(names[1])
if err != nil {
fd.NewName = names[1]
return true
case lineCount == 3 && strings.HasPrefix(fd.Extended[2], "Binary files ") || lineCount > 3 && strings.HasPrefix(fd.Extended[2], "GIT binary patch"):
names := strings.SplitN(fd.Extended[0][len("diff --git "):], " ", 2)
fd.OrigName, err = strconv.Unquote(names[0])
if err != nil {
fd.OrigName = names[0]
}
fd.NewName, err = strconv.Unquote(names[1])
if err != nil {
fd.NewName = names[1]
}
return true
default:
return false
}

if isNewFile {
fd.OrigName = "/dev/null"
}

if isDeletedFile {
fd.NewName = "/dev/null"
}

return true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this function is finally readable, thanks so much!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be much better, but I had a lot of trouble introducing names for the reused-conditions (lineCount == 3) that wouldn't be confusing, so stopped. Maybe in the future we should revisit this.

}

var (
Expand Down
3 changes: 3 additions & 0 deletions diff/testdata/sample_file_extended_empty_mode_change.diff
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
diff --git a/docs/index.md b/docs/index.md
old mode 100644
new mode 100755
6 changes: 6 additions & 0 deletions diff/testdata/sample_file_extended_empty_rename_and_mode_change.diff
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
diff --git a/textfile.txt b/textfile2.txt
old mode 100644
new mode 100755
similarity index 100%
rename from textfile.txt
rename to textfile2.txt