Skip to content

Commit 891144f

Browse files
dmitshurdmitshur
committed
WIP: Fix support for empty diffs (new, deleted, renamed 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. Improve test coverage significantly. TODO: Update .proto and regenerate .pb.go.
1 parent ba62c45 commit 891144f

10 files changed

+366
-16
lines changed

‎diff/diff.pb.go

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎diff/diff.proto

Lines changed: 5 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 {
@@ -33,6 +33,9 @@ message FileDiff {
3333

3434
// hunks that were changed from orig to new
3535
repeated Hunk Hunks = 6;
36+
37+
// TODO: Consider if this is neccessary.
38+
bool Empty = 7;
3639
}
3740

3841

‎diff/diff_test.go

Lines changed: 243 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,238 @@ 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+
Empty: true,
137+
},
138+
},
139+
{
140+
filename: "sample_file_extended_empty_deleted.diff",
141+
wantDiff: &FileDiff{
142+
OrigName: "a/vendor/go/build/testdata/empty/dummy",
143+
OrigTime: nil,
144+
NewName: "/dev/null",
145+
NewTime: nil,
146+
Extended: []string{
147+
"diff --git a/vendor/go/build/testdata/empty/dummy b/vendor/go/build/testdata/empty/dummy",
148+
"deleted file mode 100644",
149+
"index e69de29..0000000",
150+
},
151+
Empty: true,
152+
},
153+
},
154+
{
155+
filename: "sample_file_extended_empty_rename.diff",
156+
wantDiff: &FileDiff{
157+
OrigName: "a/docs/integrations/Email_Notifications.md",
158+
OrigTime: nil,
159+
NewName: "b/docs/integrations/email-notifications.md",
160+
NewTime: nil,
161+
Extended: []string{
162+
"diff --git a/docs/integrations/Email_Notifications.md b/docs/integrations/email-notifications.md",
163+
"similarity index 100%",
164+
"rename from docs/integrations/Email_Notifications.md",
165+
"rename to docs/integrations/email-notifications.md",
166+
},
167+
Empty: true,
168+
},
169+
},
170+
}
171+
for _, test := range tests {
172+
diffData, err := ioutil.ReadFile(filepath.Join("testdata", test.filename))
173+
if err != nil {
174+
t.Fatal(err)
175+
}
176+
diff, err := ParseFileDiff(diffData)
177+
if err != nil {
178+
t.Fatalf("%s: got ParseFileDiff error %v", test.filename, err)
179+
}
180+
181+
diff.Hunks = nil
182+
if got, want := diff, test.wantDiff; !reflect.DeepEqual(got, want) {
183+
t.Errorf("%s:\n\ngot: %v\nwant: %v", test.filename, goon.Sdump(got), goon.Sdump(want))
184+
}
185+
}
186+
}
187+
188+
func TestParseMultiFileDiffHeaders(t *testing.T) {
189+
tests := []struct {
190+
filename string
191+
wantDiffs []*FileDiff
192+
}{
193+
{
194+
filename: "sample_multi_file_new.diff",
195+
wantDiffs: []*FileDiff{
196+
{
197+
OrigName: "/dev/null",
198+
OrigTime: nil,
199+
NewName: "b/_vendor/go/build/syslist_test.go",
200+
NewTime: nil,
201+
Extended: []string{
202+
"diff --git a/_vendor/go/build/syslist_test.go b/_vendor/go/build/syslist_test.go",
203+
"new file mode 100644",
204+
"index 0000000..3be2928",
205+
},
206+
},
207+
{
208+
OrigName: "/dev/null",
209+
OrigTime: nil,
210+
NewName: "b/_vendor/go/build/testdata/empty/dummy",
211+
NewTime: nil,
212+
Extended: []string{
213+
"diff --git a/_vendor/go/build/testdata/empty/dummy b/_vendor/go/build/testdata/empty/dummy",
214+
"new file mode 100644",
215+
"index 0000000..e69de29",
216+
},
217+
Empty: true,
218+
},
219+
{
220+
OrigName: "/dev/null",
221+
OrigTime: nil,
222+
NewName: "b/_vendor/go/build/testdata/multi/file.go",
223+
NewTime: nil,
224+
Extended: []string{
225+
"diff --git a/_vendor/go/build/testdata/multi/file.go b/_vendor/go/build/testdata/multi/file.go",
226+
"new file mode 100644",
227+
"index 0000000..ee946eb",
228+
},
229+
},
230+
},
231+
},
232+
{
233+
filename: "sample_multi_file_deleted.diff",
234+
wantDiffs: []*FileDiff{
235+
{
236+
OrigName: "a/vendor/go/build/syslist_test.go",
237+
OrigTime: nil,
238+
NewName: "/dev/null",
239+
NewTime: nil,
240+
Extended: []string{
241+
"diff --git a/vendor/go/build/syslist_test.go b/vendor/go/build/syslist_test.go",
242+
"deleted file mode 100644",
243+
"index 3be2928..0000000",
244+
},
245+
},
246+
{
247+
OrigName: "a/vendor/go/build/testdata/empty/dummy",
248+
OrigTime: nil,
249+
NewName: "/dev/null",
250+
NewTime: nil,
251+
Extended: []string{
252+
"diff --git a/vendor/go/build/testdata/empty/dummy b/vendor/go/build/testdata/empty/dummy",
253+
"deleted file mode 100644",
254+
"index e69de29..0000000",
255+
},
256+
Empty: true,
257+
},
258+
{
259+
OrigName: "a/vendor/go/build/testdata/multi/file.go",
260+
OrigTime: nil,
261+
NewName: "/dev/null",
262+
NewTime: nil,
263+
Extended: []string{
264+
"diff --git a/vendor/go/build/testdata/multi/file.go b/vendor/go/build/testdata/multi/file.go",
265+
"deleted file mode 100644",
266+
"index ee946eb..0000000",
267+
},
268+
},
269+
},
270+
},
271+
{
272+
filename: "sample_multi_file_rename.diff",
273+
wantDiffs: []*FileDiff{
274+
{
275+
OrigName: "a/README.md",
276+
OrigTime: nil,
277+
NewName: "b/README.md",
278+
NewTime: nil,
279+
Extended: []string{
280+
"diff --git a/README.md b/README.md",
281+
"index 5f3d591..96a24fa 100644",
282+
},
283+
},
284+
{
285+
OrigName: "a/docs/integrations/Email_Notifications.md",
286+
OrigTime: nil,
287+
NewName: "b/docs/integrations/email-notifications.md",
288+
NewTime: nil,
289+
Extended: []string{
290+
"diff --git a/docs/integrations/Email_Notifications.md b/docs/integrations/email-notifications.md",
291+
"similarity index 100%",
292+
"rename from docs/integrations/Email_Notifications.md",
293+
"rename to docs/integrations/email-notifications.md",
294+
},
295+
Empty: true,
296+
},
297+
{
298+
OrigName: "a/release_notes.md",
299+
OrigTime: nil,
300+
NewName: "b/release_notes.md",
301+
NewTime: nil,
302+
Extended: []string{
303+
"diff --git a/release_notes.md b/release_notes.md",
304+
"index f2ff13f..f060cb5 100644",
305+
},
306+
},
307+
},
308+
},
309+
}
310+
for _, test := range tests {
311+
diffData, err := ioutil.ReadFile(filepath.Join("testdata", test.filename))
312+
if err != nil {
313+
t.Fatal(err)
314+
}
315+
diffs, err := ParseMultiFileDiff(diffData)
316+
if err != nil {
317+
t.Fatalf("%s: got ParseMultiFileDiff error %v", test.filename, err)
318+
}
319+
320+
for i := range diffs {
321+
diffs[i].Hunks = nil // This test focuses on things other than hunks, so don't compare them.
322+
}
323+
if got, want := diffs, test.wantDiffs; !reflect.DeepEqual(got, want) {
324+
t.Errorf("%s:\n\ngot: %v\nwant: %v", test.filename, goon.Sdump(got), goon.Sdump(want))
325+
}
326+
}
327+
}
328+
94329
func TestParseFileDiffAndPrintFileDiff(t *testing.T) {
95330
tests := []struct {
96331
filename string
@@ -99,6 +334,9 @@ func TestParseFileDiffAndPrintFileDiff(t *testing.T) {
99334
{filename: "sample_file.diff"},
100335
{filename: "sample_file_no_timestamp.diff"},
101336
{filename: "sample_file_extended.diff"},
337+
{filename: "sample_file_extended_empty_new.diff"},
338+
{filename: "sample_file_extended_empty_deleted.diff"},
339+
{filename: "sample_file_extended_empty_rename.diff"},
102340
{
103341
filename: "empty.diff",
104342
wantParseErr: &ParseError{0, 0, ErrExtendedHeadersEOF},
@@ -136,6 +374,8 @@ func TestParseMultiFileDiffAndPrintMultiFileDiff(t *testing.T) {
136374
}{
137375
{filename: "sample_multi_file.diff", wantFileDiffs: 2},
138376
{filename: "sample_multi_file_single.diff", wantFileDiffs: 1},
377+
{filename: "sample_multi_file_new.diff", wantFileDiffs: 3},
378+
{filename: "sample_multi_file_deleted.diff", wantFileDiffs: 3},
139379
{filename: "sample_multi_file_rename.diff", wantFileDiffs: 3},
140380
{filename: "long_line_multi.diff", wantFileDiffs: 3},
141381
{filename: "empty.diff", wantFileDiffs: 0},
@@ -146,7 +386,7 @@ func TestParseMultiFileDiffAndPrintMultiFileDiff(t *testing.T) {
146386
if err != nil {
147387
t.Fatal(err)
148388
}
149-
diff, err := ParseMultiFileDiff(diffData)
389+
diffs, err := ParseMultiFileDiff(diffData)
150390
if err != test.wantParseErr {
151391
t.Errorf("%s: got ParseMultiFileDiff err %v, want %v", test.filename, err, test.wantParseErr)
152392
continue
@@ -155,11 +395,11 @@ func TestParseMultiFileDiffAndPrintMultiFileDiff(t *testing.T) {
155395
continue
156396
}
157397

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

162-
printed, err := PrintMultiFileDiff(diff)
402+
printed, err := PrintMultiFileDiff(diffs)
163403
if err != nil {
164404
t.Errorf("%s: PrintMultiFileDiff: %s", test.filename, err)
165405
}

0 commit comments

Comments
 (0)