From 640c65b62891093ea6cf6552fb89d96fe6042f2f Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Tue, 5 Jan 2021 10:43:56 +0100 Subject: [PATCH 1/3] Extend sample tests, including failing test --- diff/diff_test.go | 87 +++++++++++++------ ...ample_file_extended_empty_mode_change.diff | 3 + ...extended_empty_rename_and_mode_change.diff | 6 ++ 3 files changed, 70 insertions(+), 26 deletions(-) create mode 100644 diff/testdata/sample_file_extended_empty_mode_change.diff create mode 100644 diff/testdata/sample_file_extended_empty_rename_and_mode_change.diff diff --git a/diff/diff_test.go b/diff/diff_test.go index c1bbd39..0036a93 100644 --- a/diff/diff_test.go +++ b/diff/diff_test.go @@ -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{ @@ -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{ @@ -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)) + } + }) } } @@ -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)) + } + }) } } diff --git a/diff/testdata/sample_file_extended_empty_mode_change.diff b/diff/testdata/sample_file_extended_empty_mode_change.diff new file mode 100644 index 0000000..5815cc0 --- /dev/null +++ b/diff/testdata/sample_file_extended_empty_mode_change.diff @@ -0,0 +1,3 @@ +diff --git a/docs/index.md b/docs/index.md +old mode 100644 +new mode 100755 diff --git a/diff/testdata/sample_file_extended_empty_rename_and_mode_change.diff b/diff/testdata/sample_file_extended_empty_rename_and_mode_change.diff new file mode 100644 index 0000000..bcf142f --- /dev/null +++ b/diff/testdata/sample_file_extended_empty_rename_and_mode_change.diff @@ -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 From d9b0834a371b6bbfb340ce4c4a85c5390851f756 Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Tue, 5 Jan 2021 10:50:26 +0100 Subject: [PATCH 2/3] Handle rename-and-mode-change case (verbosely for now) --- diff/parse.go | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/diff/parse.go b/diff/parse.go index b7d8cee..83b733d 100644 --- a/diff/parse.go +++ b/diff/parse.go @@ -407,10 +407,27 @@ func handleEmpty(fd *FileDiff) (wasEmpty bool) { 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 "): + case lineCount == 6 && strings.HasPrefix(fd.Extended[2], "rename from ") && strings.HasPrefix(fd.Extended[3], "rename to ") && strings.HasPrefix(fd.Extended[5], "Binary files "): names := strings.SplitN(fd.Extended[0][len("diff --git "):], " ", 2) - fd.OrigName = names[0] - fd.NewName = names[1] + 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[4], "rename from ") && strings.HasPrefix(fd.Extended[5], "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 == 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) From 3a166d4a734b373b44327b382e3c9362d87670df Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Tue, 5 Jan 2021 11:01:55 +0100 Subject: [PATCH 3/3] Simplify handling of empty file diffs --- diff/parse.go | 118 ++++++++++++++++++-------------------------------- 1 file changed, 42 insertions(+), 76 deletions(-) diff --git a/diff/parse.go b/diff/parse.go index 83b733d..bad1486 100644 --- a/diff/parse.go +++ b/diff/parse.go @@ -357,92 +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) + } + + linesHavePrefixes := func(idx1 int, prefix1 string, idx2 int, prefix2 string) bool { + return lineHasPrefix(idx1, prefix1) && lineHasPrefix(idx2, prefix2) + } + + 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] + } - names := strings.SplitN(fd.Extended[0][len("diff --git "):], " ", 2) + if isNewFile { 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 "): + } - names := strings.SplitN(fd.Extended[0][len("diff --git "):], " ", 2) - fd.OrigName, err = strconv.Unquote(names[0]) - if err != nil { - fd.OrigName = names[0] - } + if isDeletedFile { 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[2], "rename from ") && strings.HasPrefix(fd.Extended[3], "rename to ") && strings.HasPrefix(fd.Extended[5], "Binary files "): - 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[4], "rename from ") && strings.HasPrefix(fd.Extended[5], "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 == 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 } + + return true } var (