Merge branch 'fix/diff-path-handling' into 'master'

Improve path handling in diff parsing

See merge request !90

3 files changed, 66 insertions, 25 deletions

diff --git a/internal/diff/diff.go b/internal/diff/diff.go
index c5d2f5573..05bb5447d 100644
--- a/internal/diff/diff.go
+++ b/internal/diff/diff.go
@@ -32,10 +32,9 @@ type Parser struct {
 }
 
 var (
-	diffHeaderRegexp       = regexp.MustCompile(`(?m)^diff --git a/(.*?) b/(.*?)$`)
+	diffHeaderRegexp       = regexp.MustCompile(`(?m)^diff --git "?a/(.*?)"? "?b/(.*?)"?$`)
 	indexHeaderRegexp      = regexp.MustCompile(`(?m)^index ([[:xdigit:]]{40})..([[:xdigit:]]{40})(?:\s([[:digit:]]+))?$`)
-	pathHeaderRegexp       = regexp.MustCompile(`(?m)^([-+]){3} (?:[ab]/)?(.*?)$`)
-	renameCopyHeaderRegexp = regexp.MustCompile(`(?m)^(copy|rename) (from|to) (.*?)$`)
+	renameCopyHeaderRegexp = regexp.MustCompile(`(?m)^(copy|rename) (from|to) "?(.*?)"?$`)
 	modeHeaderRegexp       = regexp.MustCompile(`(?m)^(old|new|(?:deleted|new) file) mode (\d+)$`)
 )
 
@@ -65,10 +64,11 @@ func (parser *Parser) Parse() bool {
 				return false
 			}
 
-			if len(line) < 10 {
+			if len(line) > 0 && len(line) < 10 {
 				consumeChunkLine(parser.reader, parser.currentDiff)
-				return true
 			}
+
+			return true
 		} else if err != nil {
 			parser.err = fmt.Errorf("ParseDiffOutput: Unexpected error while peeking: %v", err)
 			return false
@@ -90,7 +90,7 @@ func (parser *Parser) Parse() bool {
 
 			parser.err = consumeChunkLine(parser.reader, parser.currentDiff)
 		} else if helper.ByteSliceHasAnyPrefix(line, "---", "+++") {
-			parser.err = parseHeader(parser.reader, parser.currentDiff)
+			parser.err = consumeLine(parser.reader)
 		} else if helper.ByteSliceHasAnyPrefix(line, "-", "+", " ", "\\") {
 			parser.err = consumeChunkLine(parser.reader, parser.currentDiff)
 		} else {
@@ -124,8 +124,8 @@ func parseHeader(reader *bufio.Reader, diff *Diff) error {
 	}
 
 	if matches := diffHeaderRegexp.FindSubmatch(line); len(matches) > 0 { // diff --git a/Makefile b/Makefile
-		diff.FromPath = matches[1]
-		diff.ToPath = matches[1]
+		diff.FromPath = unescapeOctalBytes(matches[1])
+		diff.ToPath = unescapeOctalBytes(matches[1])
 	}
 
 	if matches := indexHeaderRegexp.FindStringSubmatch(string(line)); len(matches) > 0 { // index a8b75d25da09b92b9f8b02151b001217ec24e0ea..3ecb2f9d50ed85f781569431df9f110bff6cb607 100644
@@ -142,21 +142,12 @@ func parseHeader(reader *bufio.Reader, diff *Diff) error {
 		}
 	}
 
-	if matches := pathHeaderRegexp.FindSubmatch(line); len(matches) > 0 { // --- a/Makefile or +++ b/Makefile
-		switch matches[1][0] {
-		case '-':
-			diff.FromPath = matches[2]
-		case '+':
-			diff.ToPath = matches[2]
-		}
-	}
-
 	if matches := renameCopyHeaderRegexp.FindSubmatch(line); len(matches) > 0 { // rename from cmd/gitaly-client/main.go
 		switch string(matches[2]) {
 		case "from":
-			diff.FromPath = matches[3]
+			diff.FromPath = unescapeOctalBytes(matches[3])
 		case "to":
-			diff.ToPath = matches[3]
+			diff.ToPath = unescapeOctalBytes(matches[3])
 		}
 	}
 
@@ -199,3 +190,32 @@ func consumeBinaryNotice(reader *bufio.Reader, diff *Diff) error {
 
 	return nil
 }
+
+func consumeLine(reader *bufio.Reader) error {
+	_, err := reader.ReadBytes('\n')
+	if err != nil && err != io.EOF {
+		return fmt.Errorf("ParseDiffOutput: Unexpected error while reading binary notice: %v", err)
+	}
+
+	return nil
+}
+
+func unescapeOctalBytes(s []byte) []byte {
+	var unescaped []byte
+
+	for i := 0; i < len(s); i++ {
+		if s[i] == '\\' && i+3 < len(s) && helper.IsNumber(s[i+1:i+4]) {
+			octalByte, err := strconv.ParseUint(string(s[i+1:i+4]), 8, 8)
+			if err == nil {
+				unescaped = append(unescaped, byte(octalByte))
+
+				i += 3
+				continue
+			}
+		}
+
+		unescaped = append(unescaped, s[i])
+	}
+
+	return unescaped
+}
diff --git a/internal/helper/byte.go b/internal/helper/byte.go
index 7049d0b75..a346db940 100644
--- a/internal/helper/byte.go
+++ b/internal/helper/byte.go
@@ -14,3 +14,13 @@ func ByteSliceHasAnyPrefix(s []byte, prefixes ...string) bool {
 
 	return false
 }
+
+// IsNumber tests whether the byte slice s contains only digits or not
+func IsNumber(s []byte) bool {
+	for i := range s {
+		if !bytes.Contains([]byte("1234567890"), s[i:i+1]) {
+			return false
+		}
+	}
+	return true
+}
diff --git a/internal/service/diff/commit_test.go b/internal/service/diff/commit_test.go
index ffd9c4dda..1cf88283a 100644
--- a/internal/service/diff/commit_test.go
+++ b/internal/service/diff/commit_test.go
@@ -26,8 +26,8 @@ func TestSuccessfulCommitDiffRequest(t *testing.T) {
 
 	client := newDiffClient(t)
 	repo := &pb.Repository{Path: path.Join(testRepoRoot, testRepo)}
-	rightCommit := "372ab6950519549b14d220271ee2322caa44d4eb"
-	leftCommit := rightCommit + "~" // Parent of rightCommit
+	rightCommit := "57290e673a4c87f51294f5216672cbc58d485d25"
+	leftCommit := rightCommit + "~~" // Second ancestor of rightCommit
 	rpcRequest := &pb.CommitDiffRequest{Repository: repo, RightCommitId: rightCommit, LeftCommitId: leftCommit}
 
 	c, err := client.CommitDiff(context.Background(), rpcRequest)
@@ -46,7 +46,7 @@ func TestSuccessfulCommitDiffRequest(t *testing.T) {
 				OldMode:  0100644,
 				NewMode:  0,
 				FromPath: []byte("gitaly/deleted-file"),
-				ToPath:   []byte("/dev/null"),
+				ToPath:   []byte("gitaly/deleted-file"),
 				Binary:   false,
 			},
 			ChunksCombined: testhelper.MustReadFile(t, "testdata/deleted-file-chunks.txt"),
@@ -104,7 +104,7 @@ func TestSuccessfulCommitDiffRequest(t *testing.T) {
 				OldMode:  0100644,
 				NewMode:  0,
 				FromPath: []byte("gitaly/named-file-with-mods"),
-				ToPath:   []byte("/dev/null"),
+				ToPath:   []byte("gitaly/named-file-with-mods"),
 				Binary:   false,
 			},
 			ChunksCombined: testhelper.MustReadFile(t, "testdata/named-file-with-mods-chunks.txt"),
@@ -115,7 +115,7 @@ func TestSuccessfulCommitDiffRequest(t *testing.T) {
 				ToID:     "b464dff7a75ccc92fbd920fd9ae66a84b9d2bf94",
 				OldMode:  0,
 				NewMode:  0100644,
-				FromPath: []byte("/dev/null"),
+				FromPath: []byte("gitaly/no-newline-at-the-end"),
 				ToPath:   []byte("gitaly/no-newline-at-the-end"),
 				Binary:   false,
 			},
@@ -138,12 +138,23 @@ func TestSuccessfulCommitDiffRequest(t *testing.T) {
 				ToID:     "3856c00e9450a51a62096327167fc43d3be62eef",
 				OldMode:  0,
 				NewMode:  0100644,
-				FromPath: []byte("/dev/null"),
+				FromPath: []byte("gitaly/renamed-file-with-mods"),
 				ToPath:   []byte("gitaly/renamed-file-with-mods"),
 				Binary:   false,
 			},
 			ChunksCombined: testhelper.MustReadFile(t, "testdata/renamed-file-with-mods-chunks.txt"),
 		},
+		{
+			Diff: diff.Diff{
+				FromID:   "0000000000000000000000000000000000000000",
+				ToID:     "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391",
+				OldMode:  0,
+				NewMode:  0100755,
+				FromPath: []byte("gitaly/テスト.txt"),
+				ToPath:   []byte("gitaly/テスト.txt"),
+				Binary:   false,
+			},
+		},
 	}
 
 	i := 0