Skip to content

Commit 82e3467

Browse files
mrnuggetmrnugget
authored
Merge pull request #58 from sourcegraph/mrn/clean-up-header-parsing
Handle rename-and-mode-change in empty file diffs & clean up code
2 parents d1e74e0 + 3a166d4 commit 82e3467

4 files changed

+112
-85
lines changed

‎diff/diff_test.go

Lines changed: 61 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,20 @@ func TestParseFileDiffHeaders(t *testing.T) {
150150
},
151151
},
152152
},
153+
{
154+
filename: "sample_file_extended_empty_mode_change.diff",
155+
wantDiff: &FileDiff{
156+
OrigName: "a/docs/index.md",
157+
OrigTime: nil,
158+
NewName: "b/docs/index.md",
159+
NewTime: nil,
160+
Extended: []string{
161+
"diff --git a/docs/index.md b/docs/index.md",
162+
"old mode 100644",
163+
"new mode 100755",
164+
},
165+
},
166+
},
153167
{
154168
filename: "sample_file_extended_empty_new_binary.diff",
155169
wantDiff: &FileDiff{
@@ -209,6 +223,23 @@ func TestParseFileDiffHeaders(t *testing.T) {
209223
},
210224
},
211225
},
226+
{
227+
filename: "sample_file_extended_empty_rename_and_mode_change.diff",
228+
wantDiff: &FileDiff{
229+
OrigName: "a/textfile.txt",
230+
OrigTime: nil,
231+
NewName: "b/textfile2.txt",
232+
NewTime: nil,
233+
Extended: []string{
234+
"diff --git a/textfile.txt b/textfile2.txt",
235+
"old mode 100644",
236+
"new mode 100755",
237+
"similarity index 100%",
238+
"rename from textfile.txt",
239+
"rename to textfile2.txt",
240+
},
241+
},
242+
},
212243
{
213244
filename: "quoted_filename.diff",
214245
wantDiff: &FileDiff{
@@ -241,19 +272,21 @@ func TestParseFileDiffHeaders(t *testing.T) {
241272
},
242273
}
243274
for _, test := range tests {
244-
diffData, err := ioutil.ReadFile(filepath.Join("testdata", test.filename))
245-
if err != nil {
246-
t.Fatal(err)
247-
}
248-
diff, err := ParseFileDiff(diffData)
249-
if err != nil {
250-
t.Fatalf("%s: got ParseFileDiff error %v", test.filename, err)
251-
}
275+
t.Run(test.filename, func(t *testing.T) {
276+
diffData, err := ioutil.ReadFile(filepath.Join("testdata", test.filename))
277+
if err != nil {
278+
t.Fatal(err)
279+
}
280+
diff, err := ParseFileDiff(diffData)
281+
if err != nil {
282+
t.Fatalf("%s: got ParseFileDiff error %v", test.filename, err)
283+
}
252284

253-
diff.Hunks = nil
254-
if got, want := diff, test.wantDiff; !cmp.Equal(got, want) {
255-
t.Errorf("%s:\n\ngot - want:\n%s", test.filename, cmp.Diff(want, got))
256-
}
285+
diff.Hunks = nil
286+
if got, want := diff, test.wantDiff; !cmp.Equal(got, want) {
287+
t.Errorf("%s:\n\ngot - want:\n%s", test.filename, cmp.Diff(want, got))
288+
}
289+
})
257290
}
258291
}
259292

@@ -672,21 +705,23 @@ func TestParseMultiFileDiffHeaders(t *testing.T) {
672705
},
673706
}
674707
for _, test := range tests {
675-
diffData, err := ioutil.ReadFile(filepath.Join("testdata", test.filename))
676-
if err != nil {
677-
t.Fatal(err)
678-
}
679-
diffs, err := ParseMultiFileDiff(diffData)
680-
if err != nil {
681-
t.Fatalf("%s: got ParseMultiFileDiff error %v", test.filename, err)
682-
}
708+
t.Run(test.filename, func(t *testing.T) {
709+
diffData, err := ioutil.ReadFile(filepath.Join("testdata", test.filename))
710+
if err != nil {
711+
t.Fatal(err)
712+
}
713+
diffs, err := ParseMultiFileDiff(diffData)
714+
if err != nil {
715+
t.Fatalf("%s: got ParseMultiFileDiff error %v", test.filename, err)
716+
}
683717

684-
for i := range diffs {
685-
diffs[i].Hunks = nil // This test focuses on things other than hunks, so don't compare them.
686-
}
687-
if got, want := diffs, test.wantDiffs; !cmp.Equal(got, want) {
688-
t.Errorf("%s:\n\ngot - want:\n%s", test.filename, cmp.Diff(want, got))
689-
}
718+
for i := range diffs {
719+
diffs[i].Hunks = nil // This test focuses on things other than hunks, so don't compare them.
720+
}
721+
if got, want := diffs, test.wantDiffs; !cmp.Equal(got, want) {
722+
t.Errorf("%s:\n\ngot - want:\n%s", test.filename, cmp.Diff(want, got))
723+
}
724+
})
690725
}
691726
}
692727

‎diff/parse.go

Lines changed: 42 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -357,75 +357,58 @@ func (r *FileDiffReader) ReadExtendedHeaders() ([]string, error) {
357357
// handleEmpty detects when FileDiff was an empty diff and will not have any hunks
358358
// that follow. It updates fd fields from the parsed extended headers.
359359
func handleEmpty(fd *FileDiff) (wasEmpty bool) {
360-
var err error
361360
lineCount := len(fd.Extended)
362361
if lineCount > 0 && !strings.HasPrefix(fd.Extended[0], "diff --git ") {
363362
return false
364363
}
365-
switch {
366-
case (lineCount == 3 || lineCount == 4 && strings.HasPrefix(fd.Extended[3], "Binary files ") || lineCount > 4 && strings.HasPrefix(fd.Extended[3], "GIT binary patch")) &&
367-
strings.HasPrefix(fd.Extended[1], "old mode ") && strings.HasPrefix(fd.Extended[2], "new mode "):
368364

369-
names := strings.SplitN(fd.Extended[0][len("diff --git "):], " ", 2)
370-
fd.OrigName, err = strconv.Unquote(names[0])
371-
if err != nil {
372-
fd.OrigName = names[0]
373-
}
374-
fd.NewName, err = strconv.Unquote(names[1])
375-
if err != nil {
376-
fd.NewName = names[1]
377-
}
378-
return true
379-
case (lineCount == 3 || lineCount == 4 && strings.HasPrefix(fd.Extended[3], "Binary files ") || lineCount > 4 && strings.HasPrefix(fd.Extended[3], "GIT binary patch")) &&
380-
strings.HasPrefix(fd.Extended[1], "new file mode "):
365+
lineHasPrefix := func(idx int, prefix string) bool {
366+
return strings.HasPrefix(fd.Extended[idx], prefix)
367+
}
381368

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

392-
names := strings.SplitN(fd.Extended[0][len("diff --git "):], " ", 2)
393-
fd.OrigName, err = strconv.Unquote(names[0])
394-
if err != nil {
395-
fd.OrigName = names[0]
396-
}
397-
fd.NewName = "/dev/null"
398-
return true
399-
case lineCount == 4 && strings.HasPrefix(fd.Extended[2], "rename from ") && strings.HasPrefix(fd.Extended[3], "rename to "):
400-
names := strings.SplitN(fd.Extended[0][len("diff --git "):], " ", 2)
401-
fd.OrigName, err = strconv.Unquote(names[0])
402-
if err != nil {
403-
fd.OrigName = names[0]
404-
}
405-
fd.NewName, err = strconv.Unquote(names[1])
406-
if err != nil {
407-
fd.NewName = names[1]
408-
}
409-
return true
410-
case lineCount == 6 && strings.HasPrefix(fd.Extended[5], "Binary files ") && strings.HasPrefix(fd.Extended[2], "rename from ") && strings.HasPrefix(fd.Extended[3], "rename to "):
411-
names := strings.SplitN(fd.Extended[0][len("diff --git "):], " ", 2)
373+
isRename := (lineCount == 4 && linesHavePrefixes(2, "rename from ", 3, "rename to ")) ||
374+
(lineCount == 6 && linesHavePrefixes(2, "rename from ", 3, "rename to ") && lineHasPrefix(5, "Binary files ")) ||
375+
(lineCount == 6 && linesHavePrefixes(1, "old mode ", 2, "new mode ") && linesHavePrefixes(4, "rename from ", 5, "rename to "))
376+
377+
isDeletedFile := (lineCount == 3 || lineCount == 4 && lineHasPrefix(3, "Binary files ") || lineCount > 4 && lineHasPrefix(3, "GIT binary patch")) &&
378+
lineHasPrefix(1, "deleted file mode ")
379+
380+
isNewFile := (lineCount == 3 || lineCount == 4 && lineHasPrefix(3, "Binary files ") || lineCount > 4 && lineHasPrefix(3, "GIT binary patch")) &&
381+
lineHasPrefix(1, "new file mode ")
382+
383+
isModeChange := lineCount == 3 && linesHavePrefixes(1, "old mode ", 2, "new mode ")
384+
385+
isBinaryPatch := lineCount == 3 && lineHasPrefix(2, "Binary files ") || lineCount > 3 && lineHasPrefix(2, "GIT binary patch")
386+
387+
if !isModeChange && !isRename && !isBinaryPatch && !isNewFile && !isDeletedFile {
388+
return false
389+
}
390+
391+
names := strings.SplitN(fd.Extended[0][len("diff --git "):], " ", 2)
392+
393+
var err error
394+
fd.OrigName, err = strconv.Unquote(names[0])
395+
if err != nil {
412396
fd.OrigName = names[0]
397+
}
398+
fd.NewName, err = strconv.Unquote(names[1])
399+
if err != nil {
413400
fd.NewName = names[1]
414-
return true
415-
case lineCount == 3 && strings.HasPrefix(fd.Extended[2], "Binary files ") || lineCount > 3 && strings.HasPrefix(fd.Extended[2], "GIT binary patch"):
416-
names := strings.SplitN(fd.Extended[0][len("diff --git "):], " ", 2)
417-
fd.OrigName, err = strconv.Unquote(names[0])
418-
if err != nil {
419-
fd.OrigName = names[0]
420-
}
421-
fd.NewName, err = strconv.Unquote(names[1])
422-
if err != nil {
423-
fd.NewName = names[1]
424-
}
425-
return true
426-
default:
427-
return false
428401
}
402+
403+
if isNewFile {
404+
fd.OrigName = "/dev/null"
405+
}
406+
407+
if isDeletedFile {
408+
fd.NewName = "/dev/null"
409+
}
410+
411+
return true
429412
}
430413

431414
var (

‎diff/testdata/sample_file_extended_empty_mode_change.diff

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
diff --git a/docs/index.md b/docs/index.md
2+
old mode 100644
3+
new mode 100755

‎diff/testdata/sample_file_extended_empty_rename_and_mode_change.diff

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
diff --git a/textfile.txt b/textfile2.txt
2+
old mode 100644
3+
new mode 100755
4+
similarity index 100%
5+
rename from textfile.txt
6+
rename to textfile2.txt

0 commit comments

Comments
 (0)