Skip to content

Commit 28094f6

Browse files
kwi-dkkwi-dk
committed
Address PR feedback
1 parent 953267f commit 28094f6

File tree

3 files changed

+133
-122
lines changed

3 files changed

+133
-122
lines changed

‎diff/diff_test.go

-79
Original file line numberDiff line numberDiff line change
@@ -29,85 +29,6 @@ func init() {
2929
time.Local = time.UTC
3030
}
3131

32-
func TestReadQuotedFilename_Success(t *testing.T) {
33-
tests := []string{
34-
`""`, "", "",
35-
`"aaa"`, "aaa", "",
36-
`"aaa" bbb`, "aaa", " bbb",
37-
`"\""`, "\"", "",
38-
`"uh \"oh\""`, "uh \"oh\"", "",
39-
`"uh \\"oh\\""`, "uh \\", `oh\\""`,
40-
`"uh \\\"oh\\\""`, "uh \\\"oh\\\"", "",
41-
}
42-
for i := 0; i < len(tests); i += 3 {
43-
input := tests[i]
44-
value, remainder, err := readQuotedFilename(input)
45-
if err != nil {
46-
t.Errorf("readQuotedFilename(`%s`): expected success, got '%s'", input, err)
47-
} else if value != tests[i+1] || remainder != tests[i+2] {
48-
t.Errorf("readQuotedFilename(`%s`): expected `%s` and `%s`, got `%s` and `%s`", input, tests[i+1], tests[i+2], value, remainder)
49-
}
50-
}
51-
}
52-
53-
func TestReadQuotedFilename_Error(t *testing.T) {
54-
tests := []string{
55-
`"`,
56-
`"\"`,
57-
`"\xxx"`,
58-
}
59-
for _, input := range tests {
60-
_, _, err := readQuotedFilename(input)
61-
if err == nil {
62-
t.Errorf("readQuotedFilename(`%s`): expected error", input)
63-
}
64-
}
65-
}
66-
67-
func TestParseDiffGitArgs_Success(t *testing.T) {
68-
tests := []string{
69-
`aaa bbb`, "aaa", "bbb",
70-
`"aaa" bbb`, "aaa", "bbb",
71-
`aaa "bbb"`, "aaa", "bbb",
72-
`"aaa" "bbb"`, "aaa", "bbb",
73-
`1/a 2/z`, "1/a", "2/z",
74-
`1/hello world 2/hello world`, "1/hello world", "2/hello world",
75-
`"new\nline" and spaces`, "new\nline", "and spaces",
76-
`a/existing file with spaces "b/new, complicated\nfilen\303\270me"`, "a/existing file with spaces", "b/new, complicated\nfilen\303\270me",
77-
}
78-
for i := 0; i < len(tests); i += 3 {
79-
input := tests[i]
80-
success, first, second := parseDiffGitArgs(input)
81-
if !success {
82-
t.Errorf("`diff --git %s`: expected success", input)
83-
} else if first != tests[i+1] || second != tests[i+2] {
84-
t.Errorf("`diff --git %s`: expected `%s` and `%s`, got `%s` and `%s`", input, tests[i+1], tests[i+2], first, second)
85-
}
86-
}
87-
}
88-
89-
func TestParseDiffGitArgs_Unsuccessful(t *testing.T) {
90-
tests := []string{
91-
``,
92-
`hello_world.txt`,
93-
`word `,
94-
` word`,
95-
`"a/bad_quoting b/bad_quoting`,
96-
`a/bad_quoting "b/bad_quoting`,
97-
`a/bad_quoting b/bad_quoting"`,
98-
`"a/bad_quoting b/bad_quoting"`,
99-
`"a/bad""b/bad"`,
100-
`"a/bad" "b/bad" "c/bad"`,
101-
`a/bad "b/bad" "c/bad"`,
102-
}
103-
for _, input := range tests {
104-
success, first, second := parseDiffGitArgs(input)
105-
if success {
106-
t.Errorf("`diff --git %s`: expected unsuccessful; got `%s` and `%s`", input, first, second)
107-
}
108-
}
109-
}
110-
11132
func TestParseHunkNoChunksize(t *testing.T) {
11233
filename := "sample_no_chunksize.diff"
11334
diffData, err := ioutil.ReadFile(filepath.Join("testdata", filename))

‎diff/parse.go

+40-43
Original file line numberDiff line numberDiff line change
@@ -375,99 +375,97 @@ func (r *FileDiffReader) ReadExtendedHeaders() ([]string, error) {
375375
// readQuotedFilename extracts a quoted filename from the beginning of a string,
376376
// returning the unquoted filename and any remaining text after the filename.
377377
func readQuotedFilename(text string) (value string, remainder string, err error) {
378-
if text[0] != '"' {
379-
panic("caller must ensure filename is quoted! " + text)
378+
if text == "" || text[0] != '"' {
379+
return "", "", fmt.Errorf(`string must start with a '"': %s`, text)
380380
}
381381

382382
// The end quote is the first quote NOT preceeded by an uneven number of backslashes.
383-
var i, j int
384-
for i = 1; i < len(text); i++ {
385-
if text[i] == '"' {
386-
// walk backwards and find first non-backslash
387-
for j = i - 1; j > 0 && text[j] == '\\'; j-- {
388-
}
389-
numberOfBackslashes := i - j - 1
390-
if numberOfBackslashes%2 == 0 {
391-
break
392-
}
383+
numberOfBackslashes := 0
384+
for i, c := range text {
385+
if c == '"' && i > 0 && numberOfBackslashes%2 == 0 {
386+
value, err = strconv.Unquote(text[:i+1])
387+
remainder = text[i+1:]
388+
return
389+
} else if c == '\\' {
390+
numberOfBackslashes++
391+
} else {
392+
numberOfBackslashes = 0
393393
}
394394
}
395-
if i == len(text) {
396-
return "", "", fmt.Errorf(`end of string found while searching for '"': %s`, text)
397-
}
398-
399-
value, err = strconv.Unquote(text[:i+1])
400-
remainder = text[i+1:]
401-
return
395+
return "", "", fmt.Errorf(`end of string found while searching for '"': %s`, text)
402396
}
403397

404398
// parseDiffGitArgs extracts the two filenames from a 'diff --git' line.
405-
func parseDiffGitArgs(diffArgs string) (bool, string, string) {
399+
// Returns false on syntax error, true if syntax is valid. Even with a
400+
// valid syntax, it may be impossible to extract filenames; if so, the
401+
// function returns ("", "", true).
402+
func parseDiffGitArgs(diffArgs string) (string, string, bool) {
406403
length := len(diffArgs)
407404
if length < 3 {
408-
return false, "", ""
405+
return "", "", false
409406
}
410407

411408
if diffArgs[0] != '"' && diffArgs[length-1] != '"' {
412409
// Both filenames are unquoted.
413410
firstSpace := strings.IndexByte(diffArgs, ' ')
414411
if firstSpace <= 0 || firstSpace == length-1 {
415-
return false, "", ""
412+
return "", "", false
416413
}
417414

418415
secondSpace := strings.IndexByte(diffArgs[firstSpace+1:], ' ')
419416
if secondSpace == -1 {
420417
if diffArgs[firstSpace+1] == '"' {
421-
return false, "", ""
418+
// The second filename begins with '"', but doesn't end with one.
419+
return "", "", false
422420
}
423-
return true, diffArgs[:firstSpace], diffArgs[firstSpace+1:]
421+
return diffArgs[:firstSpace], diffArgs[firstSpace+1:], true
424422
}
425423

426424
// One or both filenames contain a space, but the names are
427425
// unquoted. Here, the 'diff --git' syntax is ambiguous, and
428426
// we have to obtain the filenames elsewhere (e.g. from the
429-
// chunk headers or extended headers). HOWEVER, if the file
427+
// hunk headers or extended headers). HOWEVER, if the file
430428
// is newly created and empty, there IS no other place to
431429
// find the filename. In this case, the two filenames are
432430
// identical (except for the leading 'a/' prefix), and we have
433431
// to handle that case here.
434432
first := diffArgs[:length/2]
435433
second := diffArgs[length/2+1:]
436434
if len(first) >= 3 && length%2 == 1 && first[1] == '/' && first[1:] == second[1:] {
437-
return true, first, second
435+
return first, second, true
438436
}
439437

440438
// The syntax is (unfortunately) valid, but we could not extract
441439
// the filenames.
442-
return true, "", ""
440+
return "", "", true
443441
}
444442

445443
if diffArgs[0] == '"' {
446444
first, remainder, err := readQuotedFilename(diffArgs)
447445
if err != nil || len(remainder) < 2 || remainder[0] != ' ' {
448-
return false, "", ""
446+
return "", "", false
449447
}
450448
if remainder[1] == '"' {
451449
second, remainder, err := readQuotedFilename(remainder[1:])
452450
if remainder != "" || err != nil {
453-
return false, "", ""
451+
return "", "", false
454452
}
455-
return true, first, second
453+
return first, second, true
456454
}
457-
return true, first, remainder[1:]
455+
return first, remainder[1:], true
458456
}
459457

460458
// In this case, second argument MUST be quoted (or it's a syntax error)
461459
i := strings.IndexByte(diffArgs, '"')
462460
if i == -1 || i+2 >= length || diffArgs[i-1] != ' ' {
463-
return false, "", ""
461+
return "", "", false
464462
}
465463

466464
second, remainder, err := readQuotedFilename(diffArgs[i:])
467465
if remainder != "" || err != nil {
468-
return false, "", ""
466+
return "", "", false
469467
}
470-
return true, diffArgs[:i-1], second
468+
return diffArgs[:i-1], second, true
471469
}
472470

473471
// handleEmpty detects when FileDiff was an empty diff and will not have any hunks
@@ -509,7 +507,7 @@ func handleEmpty(fd *FileDiff) (wasEmpty bool) {
509507
}
510508

511509
var success bool
512-
success, fd.OrigName, fd.NewName = parseDiffGitArgs(fd.Extended[0][len("diff --git "):])
510+
fd.OrigName, fd.NewName, success = parseDiffGitArgs(fd.Extended[0][len("diff --git "):])
513511
if isNewFile {
514512
fd.OrigName = "/dev/null"
515513
}
@@ -522,9 +520,9 @@ func handleEmpty(fd *FileDiff) (wasEmpty bool) {
522520
if success && (isCopy || isRename) && fd.OrigName == "" && fd.NewName == "" {
523521
diffArgs := fd.Extended[0][len("diff --git "):]
524522

525-
reconstruct := func(header string, prefix string, whichFile int, result *string) bool {
523+
tryReconstruct := func(header string, prefix string, whichFile int, result *string) {
526524
if !strings.HasPrefix(header, prefix) {
527-
return false
525+
return
528526
}
529527
rawFilename := header[len(prefix):]
530528

@@ -536,18 +534,17 @@ func handleEmpty(fd *FileDiff) (wasEmpty bool) {
536534
prefixLetterIndex = len(diffArgs) - len(rawFilename) - 2
537535
}
538536
if prefixLetterIndex < 0 || diffArgs[prefixLetterIndex+1] != '/' {
539-
return false
537+
return
540538
}
541539

542540
*result = diffArgs[prefixLetterIndex:prefixLetterIndex+2] + rawFilename
543-
return true
544541
}
545542

546543
for _, header := range fd.Extended {
547-
_ = reconstruct(header, "copy from ", 1, &fd.OrigName) ||
548-
reconstruct(header, "copy to ", 2, &fd.NewName) ||
549-
reconstruct(header, "rename from ", 1, &fd.OrigName) ||
550-
reconstruct(header, "rename to ", 2, &fd.NewName)
544+
tryReconstruct(header, "copy from ", 1, &fd.OrigName)
545+
tryReconstruct(header, "copy to ", 2, &fd.NewName)
546+
tryReconstruct(header, "rename from ", 1, &fd.OrigName)
547+
tryReconstruct(header, "rename to ", 2, &fd.NewName)
551548
}
552549
}
553550
return success

‎diff/parse_test.go

+93
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
package diff
2+
3+
import (
4+
"testing"
5+
)
6+
7+
func TestReadQuotedFilename_Success(t *testing.T) {
8+
tests := []struct {
9+
input, value, remainder string
10+
}{
11+
{input: `""`, value: "", remainder: ""},
12+
{input: `"aaa"`, value: "aaa", remainder: ""},
13+
{input: `"aaa" bbb`, value: "aaa", remainder: " bbb"},
14+
{input: `"aaa" "bbb" ccc`, value: "aaa", remainder: ` "bbb" ccc`},
15+
{input: `"\""`, value: "\"", remainder: ""},
16+
{input: `"uh \"oh\""`, value: "uh \"oh\"", remainder: ""},
17+
{input: `"uh \\"oh\\""`, value: "uh \\", remainder: `oh\\""`},
18+
{input: `"uh \\\"oh\\\""`, value: "uh \\\"oh\\\"", remainder: ""},
19+
}
20+
for _, tc := range tests {
21+
value, remainder, err := readQuotedFilename(tc.input)
22+
if err != nil {
23+
t.Errorf("readQuotedFilename(`%s`): expected success, got '%s'", tc.input, err)
24+
} else if value != tc.value || remainder != tc.remainder {
25+
t.Errorf("readQuotedFilename(`%s`): expected `%s` and `%s`, got `%s` and `%s`", tc.input, tc.value, tc.remainder, value, remainder)
26+
}
27+
}
28+
}
29+
30+
func TestReadQuotedFilename_Error(t *testing.T) {
31+
tests := []string{
32+
// Doesn't start with a quote
33+
``,
34+
`foo`,
35+
` "foo"`,
36+
// Missing end quote
37+
`"`,
38+
`"\"`,
39+
// "\x" is not a valid Go string literal escape
40+
`"\xxx"`,
41+
}
42+
for _, input := range tests {
43+
_, _, err := readQuotedFilename(input)
44+
if err == nil {
45+
t.Errorf("readQuotedFilename(`%s`): expected error", input)
46+
}
47+
}
48+
}
49+
50+
func TestParseDiffGitArgs_Success(t *testing.T) {
51+
tests := []struct {
52+
input, first, second string
53+
}{
54+
{input: `aaa bbb`, first: "aaa", second: "bbb"},
55+
{input: `"aaa" bbb`, first: "aaa", second: "bbb"},
56+
{input: `aaa "bbb"`, first: "aaa", second: "bbb"},
57+
{input: `"aaa" "bbb"`, first: "aaa", second: "bbb"},
58+
{input: `1/a 2/z`, first: "1/a", second: "2/z"},
59+
{input: `1/hello world 2/hello world`, first: "1/hello world", second: "2/hello world"},
60+
{input: `"new\nline" and spaces`, first: "new\nline", second: "and spaces"},
61+
{input: `a/existing file with spaces "b/new, complicated\nfilen\303\270me"`, first: "a/existing file with spaces", second: "b/new, complicated\nfilen\303\270me"},
62+
}
63+
for _, tc := range tests {
64+
first, second, success := parseDiffGitArgs(tc.input)
65+
if !success {
66+
t.Errorf("`diff --git %s`: expected success", tc.input)
67+
} else if first != tc.first || second != tc.second {
68+
t.Errorf("`diff --git %s`: expected `%s` and `%s`, got `%s` and `%s`", tc.input, tc.first, tc.second, first, second)
69+
}
70+
}
71+
}
72+
73+
func TestParseDiffGitArgs_Unsuccessful(t *testing.T) {
74+
tests := []string{
75+
``,
76+
`hello_world.txt`,
77+
`word `,
78+
` word`,
79+
`"a/bad_quoting b/bad_quoting`,
80+
`a/bad_quoting "b/bad_quoting`,
81+
`a/bad_quoting b/bad_quoting"`,
82+
`"a/bad_quoting b/bad_quoting"`,
83+
`"a/bad""b/bad"`,
84+
`"a/bad" "b/bad" "c/bad"`,
85+
`a/bad "b/bad" "c/bad"`,
86+
}
87+
for _, input := range tests {
88+
first, second, success := parseDiffGitArgs(input)
89+
if success {
90+
t.Errorf("`diff --git %s`: expected unsuccessful; got `%s` and `%s`", input, first, second)
91+
}
92+
}
93+
}

0 commit comments

Comments
 (0)