Skip to content

Commit 9bc7549

Browse files
dmitshurdmitshur
committed
Fix support for empty diffs (new, deleted, renamed, binary files).
There are situations where a diff is empty. For example, if a blank file is added a removed. Or if a file is renamed without any changes. This change detects and handles those situations properly. That includes fixing support for modified binary files. Improve test coverage significantly. Fixes #10.
1 parent 0581ef9 commit 9bc7549

12 files changed

+431
-25
lines changed

‎diff/diff.proto

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,9 @@ option (gogoproto.marshaler_all) = true;
1010
option (gogoproto.sizer_all) = true;
1111

1212
// A FileDiff represents a unified diff for a single file.
13-
//
13+
//
1414
// A file unified diff has a header that resembles the following:
15-
//
15+
//
1616
// --- oldname 2009-10-11 15:12:20.000000000 -0700
1717
// +++ newname 2009-10-11 15:12:30.000000000 -0700
1818
message FileDiff {

‎diff/diff_test.go

Lines changed: 275 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,9 @@ import (
88
"strings"
99
"testing"
1010
"time"
11+
12+
"github.com/shurcooL/go-goon"
13+
"sourcegraph.com/sqs/pbtypes"
1114
)
1215

1316
func init() {
@@ -91,6 +94,268 @@ func TestParseHunksAndPrintHunks(t *testing.T) {
9194
}
9295
}
9396

97+
func TestParseFileDiffHeaders(t *testing.T) {
98+
tests := []struct {
99+
filename string
100+
wantDiff *FileDiff
101+
}{
102+
{
103+
filename: "sample_file.diff",
104+
wantDiff: &FileDiff{
105+
OrigName: "oldname",
106+
OrigTime: &pbtypes.Timestamp{Seconds: 1255273940},
107+
NewName: "newname",
108+
NewTime: &pbtypes.Timestamp{Seconds: 1255273950},
109+
},
110+
},
111+
{
112+
filename: "sample_file_extended.diff",
113+
wantDiff: &FileDiff{
114+
OrigName: "oldname",
115+
OrigTime: &pbtypes.Timestamp{Seconds: 1255273940},
116+
NewName: "newname",
117+
NewTime: &pbtypes.Timestamp{Seconds: 1255273950},
118+
Extended: []string{
119+
"diff --git a/vcs/git_cmd.go b/vcs/git_cmd.go",
120+
"index aa4de15..7c048ab 100644",
121+
},
122+
},
123+
},
124+
{
125+
filename: "sample_file_extended_empty_new.diff",
126+
wantDiff: &FileDiff{
127+
OrigName: "/dev/null",
128+
OrigTime: nil,
129+
NewName: "b/vendor/go/build/testdata/empty/dummy",
130+
NewTime: nil,
131+
Extended: []string{
132+
"diff --git a/vendor/go/build/testdata/empty/dummy b/vendor/go/build/testdata/empty/dummy",
133+
"new file mode 100644",
134+
"index 0000000..e69de29",
135+
},
136+
},
137+
},
138+
{
139+
filename: "sample_file_extended_empty_deleted.diff",
140+
wantDiff: &FileDiff{
141+
OrigName: "a/vendor/go/build/testdata/empty/dummy",
142+
OrigTime: nil,
143+
NewName: "/dev/null",
144+
NewTime: nil,
145+
Extended: []string{
146+
"diff --git a/vendor/go/build/testdata/empty/dummy b/vendor/go/build/testdata/empty/dummy",
147+
"deleted file mode 100644",
148+
"index e69de29..0000000",
149+
},
150+
},
151+
},
152+
{
153+
filename: "sample_file_extended_empty_rename.diff",
154+
wantDiff: &FileDiff{
155+
OrigName: "a/docs/integrations/Email_Notifications.md",
156+
OrigTime: nil,
157+
NewName: "b/docs/integrations/email-notifications.md",
158+
NewTime: nil,
159+
Extended: []string{
160+
"diff --git a/docs/integrations/Email_Notifications.md b/docs/integrations/email-notifications.md",
161+
"similarity index 100%",
162+
"rename from docs/integrations/Email_Notifications.md",
163+
"rename to docs/integrations/email-notifications.md",
164+
},
165+
},
166+
},
167+
}
168+
for _, test := range tests {
169+
diffData, err := ioutil.ReadFile(filepath.Join("testdata", test.filename))
170+
if err != nil {
171+
t.Fatal(err)
172+
}
173+
diff, err := ParseFileDiff(diffData)
174+
if err != nil {
175+
t.Fatalf("%s: got ParseFileDiff error %v", test.filename, err)
176+
}
177+
178+
diff.Hunks = nil
179+
if got, want := diff, test.wantDiff; !reflect.DeepEqual(got, want) {
180+
t.Errorf("%s:\n\ngot: %v\nwant: %v", test.filename, goon.Sdump(got), goon.Sdump(want))
181+
}
182+
}
183+
}
184+
185+
func TestParseMultiFileDiffHeaders(t *testing.T) {
186+
tests := []struct {
187+
filename string
188+
wantDiffs []*FileDiff
189+
}{
190+
{
191+
filename: "sample_multi_file_new.diff",
192+
wantDiffs: []*FileDiff{
193+
{
194+
OrigName: "/dev/null",
195+
OrigTime: nil,
196+
NewName: "b/_vendor/go/build/syslist_test.go",
197+
NewTime: nil,
198+
Extended: []string{
199+
"diff --git a/_vendor/go/build/syslist_test.go b/_vendor/go/build/syslist_test.go",
200+
"new file mode 100644",
201+
"index 0000000..3be2928",
202+
},
203+
},
204+
{
205+
OrigName: "/dev/null",
206+
OrigTime: nil,
207+
NewName: "b/_vendor/go/build/testdata/empty/dummy",
208+
NewTime: nil,
209+
Extended: []string{
210+
"diff --git a/_vendor/go/build/testdata/empty/dummy b/_vendor/go/build/testdata/empty/dummy",
211+
"new file mode 100644",
212+
"index 0000000..e69de29",
213+
},
214+
},
215+
{
216+
OrigName: "/dev/null",
217+
OrigTime: nil,
218+
NewName: "b/_vendor/go/build/testdata/multi/file.go",
219+
NewTime: nil,
220+
Extended: []string{
221+
"diff --git a/_vendor/go/build/testdata/multi/file.go b/_vendor/go/build/testdata/multi/file.go",
222+
"new file mode 100644",
223+
"index 0000000..ee946eb",
224+
},
225+
},
226+
},
227+
},
228+
{
229+
filename: "sample_multi_file_deleted.diff",
230+
wantDiffs: []*FileDiff{
231+
{
232+
OrigName: "a/vendor/go/build/syslist_test.go",
233+
OrigTime: nil,
234+
NewName: "/dev/null",
235+
NewTime: nil,
236+
Extended: []string{
237+
"diff --git a/vendor/go/build/syslist_test.go b/vendor/go/build/syslist_test.go",
238+
"deleted file mode 100644",
239+
"index 3be2928..0000000",
240+
},
241+
},
242+
{
243+
OrigName: "a/vendor/go/build/testdata/empty/dummy",
244+
OrigTime: nil,
245+
NewName: "/dev/null",
246+
NewTime: nil,
247+
Extended: []string{
248+
"diff --git a/vendor/go/build/testdata/empty/dummy b/vendor/go/build/testdata/empty/dummy",
249+
"deleted file mode 100644",
250+
"index e69de29..0000000",
251+
},
252+
},
253+
{
254+
OrigName: "a/vendor/go/build/testdata/multi/file.go",
255+
OrigTime: nil,
256+
NewName: "/dev/null",
257+
NewTime: nil,
258+
Extended: []string{
259+
"diff --git a/vendor/go/build/testdata/multi/file.go b/vendor/go/build/testdata/multi/file.go",
260+
"deleted file mode 100644",
261+
"index ee946eb..0000000",
262+
},
263+
},
264+
},
265+
},
266+
{
267+
filename: "sample_multi_file_rename.diff",
268+
wantDiffs: []*FileDiff{
269+
{
270+
OrigName: "a/README.md",
271+
OrigTime: nil,
272+
NewName: "b/README.md",
273+
NewTime: nil,
274+
Extended: []string{
275+
"diff --git a/README.md b/README.md",
276+
"index 5f3d591..96a24fa 100644",
277+
},
278+
},
279+
{
280+
OrigName: "a/docs/integrations/Email_Notifications.md",
281+
OrigTime: nil,
282+
NewName: "b/docs/integrations/email-notifications.md",
283+
NewTime: nil,
284+
Extended: []string{
285+
"diff --git a/docs/integrations/Email_Notifications.md b/docs/integrations/email-notifications.md",
286+
"similarity index 100%",
287+
"rename from docs/integrations/Email_Notifications.md",
288+
"rename to docs/integrations/email-notifications.md",
289+
},
290+
},
291+
{
292+
OrigName: "a/release_notes.md",
293+
OrigTime: nil,
294+
NewName: "b/release_notes.md",
295+
NewTime: nil,
296+
Extended: []string{
297+
"diff --git a/release_notes.md b/release_notes.md",
298+
"index f2ff13f..f060cb5 100644",
299+
},
300+
},
301+
},
302+
},
303+
{
304+
filename: "sample_multi_file_binary.diff",
305+
wantDiffs: []*FileDiff{
306+
{
307+
OrigName: "a/README.md",
308+
OrigTime: nil,
309+
NewName: "b/README.md",
310+
NewTime: nil,
311+
Extended: []string{
312+
"diff --git a/README.md b/README.md",
313+
"index 7b73e04..36cde13 100644",
314+
},
315+
},
316+
{
317+
OrigName: "a/data/Font.png",
318+
OrigTime: nil,
319+
NewName: "b/data/Font.png",
320+
NewTime: nil,
321+
Extended: []string{
322+
"diff --git a/data/Font.png b/data/Font.png",
323+
"index 17a971d..599f8dd 100644",
324+
"Binary files a/data/Font.png and b/data/Font.png differ",
325+
},
326+
},
327+
{
328+
OrigName: "a/main.go",
329+
OrigTime: nil,
330+
NewName: "b/main.go",
331+
NewTime: nil,
332+
Extended: []string{
333+
"diff --git a/main.go b/main.go",
334+
"index 1aced1e..98a982e 100644",
335+
},
336+
},
337+
},
338+
},
339+
}
340+
for _, test := range tests {
341+
diffData, err := ioutil.ReadFile(filepath.Join("testdata", test.filename))
342+
if err != nil {
343+
t.Fatal(err)
344+
}
345+
diffs, err := ParseMultiFileDiff(diffData)
346+
if err != nil {
347+
t.Fatalf("%s: got ParseMultiFileDiff error %v", test.filename, err)
348+
}
349+
350+
for i := range diffs {
351+
diffs[i].Hunks = nil // This test focuses on things other than hunks, so don't compare them.
352+
}
353+
if got, want := diffs, test.wantDiffs; !reflect.DeepEqual(got, want) {
354+
t.Errorf("%s:\n\ngot: %v\nwant: %v", test.filename, goon.Sdump(got), goon.Sdump(want))
355+
}
356+
}
357+
}
358+
94359
func TestParseFileDiffAndPrintFileDiff(t *testing.T) {
95360
tests := []struct {
96361
filename string
@@ -99,6 +364,10 @@ func TestParseFileDiffAndPrintFileDiff(t *testing.T) {
99364
{filename: "sample_file.diff"},
100365
{filename: "sample_file_no_timestamp.diff"},
101366
{filename: "sample_file_extended.diff"},
367+
{filename: "sample_file_extended_empty_new.diff"},
368+
{filename: "sample_file_extended_empty_deleted.diff"},
369+
{filename: "sample_file_extended_empty_rename.diff"},
370+
{filename: "sample_file_extended_empty_binary.diff"},
102371
{
103372
filename: "empty.diff",
104373
wantParseErr: &ParseError{0, 0, ErrExtendedHeadersEOF},
@@ -136,7 +405,10 @@ func TestParseMultiFileDiffAndPrintMultiFileDiff(t *testing.T) {
136405
}{
137406
{filename: "sample_multi_file.diff", wantFileDiffs: 2},
138407
{filename: "sample_multi_file_single.diff", wantFileDiffs: 1},
408+
{filename: "sample_multi_file_new.diff", wantFileDiffs: 3},
409+
{filename: "sample_multi_file_deleted.diff", wantFileDiffs: 3},
139410
{filename: "sample_multi_file_rename.diff", wantFileDiffs: 3},
411+
{filename: "sample_multi_file_binary.diff", wantFileDiffs: 3},
140412
{filename: "long_line_multi.diff", wantFileDiffs: 3},
141413
{filename: "empty.diff", wantFileDiffs: 0},
142414
{filename: "empty_multi.diff", wantFileDiffs: 2},
@@ -146,7 +418,7 @@ func TestParseMultiFileDiffAndPrintMultiFileDiff(t *testing.T) {
146418
if err != nil {
147419
t.Fatal(err)
148420
}
149-
diff, err := ParseMultiFileDiff(diffData)
421+
diffs, err := ParseMultiFileDiff(diffData)
150422
if err != test.wantParseErr {
151423
t.Errorf("%s: got ParseMultiFileDiff err %v, want %v", test.filename, err, test.wantParseErr)
152424
continue
@@ -155,11 +427,11 @@ func TestParseMultiFileDiffAndPrintMultiFileDiff(t *testing.T) {
155427
continue
156428
}
157429

158-
if got, want := len(diff), test.wantFileDiffs; got != want {
430+
if got, want := len(diffs), test.wantFileDiffs; got != want {
159431
t.Errorf("%s: got %v instances of diff.FileDiff, expected %v", test.filename, got, want)
160432
}
161433

162-
printed, err := PrintMultiFileDiff(diff)
434+
printed, err := PrintMultiFileDiff(diffs)
163435
if err != nil {
164436
t.Errorf("%s: PrintMultiFileDiff: %s", test.filename, err)
165437
}

0 commit comments

Comments
 (0)