Skip to content

Commit

Permalink
Handle unquoted comment patch files
Browse files Browse the repository at this point in the history 
When making comment patches unfortunately the patch does not always quote the filename
This makes the diff --git header ambiguous again.

This PR finally adds handling for ambiguity in to parse patch

Fix #14812

Signed-off-by: Andrew Thornton <[email protected]>
  • Loading branch information
@zeripath
zeripath committed Feb 27, 2021
1 parent a18cdea commit bc2d0f3
Show file tree
Hide file tree
Showing 2 changed files with 157 additions and 14 deletions.
95 changes: 89 additions & 6 deletions services/gitdiff/gitdiff.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -583,6 +583,7 @@ type DiffFile struct {
IsBin bool
IsLFSFile bool
IsRenamed bool
IsAmbiguous bool
IsSubmodule bool
Sections []*DiffSection
IsIncomplete bool
Expand Down Expand Up @@ -776,12 +777,32 @@ parsingLoop:
if strings.HasSuffix(line, " 160000\n") {
curFile.IsSubmodule = true
}
case strings.HasPrefix(line, "rename from "):
curFile.IsRenamed = true
curFile.Type = DiffFileRename
if curFile.IsAmbiguous {
curFile.OldName = line[len("rename from ") : len(line)-1]
}
case strings.HasPrefix(line, "rename to "):
curFile.IsRenamed = true
curFile.Type = DiffFileRename
if curFile.IsAmbiguous {
curFile.Name = line[len("rename to ") : len(line)-1]
curFile.IsAmbiguous = false
}
case strings.HasPrefix(line, "copy from "):
curFile.IsRenamed = true
curFile.Type = DiffFileCopy
if curFile.IsAmbiguous {
curFile.OldName = line[len("copy from ") : len(line)-1]
}
case strings.HasPrefix(line, "copy to "):
curFile.IsRenamed = true
curFile.Type = DiffFileCopy
if curFile.IsAmbiguous {
curFile.Name = line[len("copy to ") : len(line)-1]
curFile.IsAmbiguous = false
}
case strings.HasPrefix(line, "new file"):
curFile.Type = DiffFileAdd
curFile.IsCreated = true
Expand All @@ -803,9 +824,35 @@ parsingLoop:
case strings.HasPrefix(line, "Binary"):
curFile.IsBin = true
case strings.HasPrefix(line, "--- "):
// Do nothing with this line
// Handle ambiguous filenames
if curFile.IsAmbiguous {
if len(line) > 6 && line[4] == 'a' {
curFile.OldName = line[6 : len(line)-1]
if line[len(line)-2] == '\t' {
curFile.OldName = curFile.OldName[:len(curFile.OldName)-1]
}
} else {
curFile.OldName = ""
}
}
// Otherwise do nothing with this line
case strings.HasPrefix(line, "+++ "):
// Do nothing with this line
// Handle ambiguous filenames
if curFile.IsAmbiguous {
if len(line) > 6 && line[4] == 'b' {
curFile.Name = line[6 : len(line)-1]
if line[len(line)-2] == '\t' {
curFile.Name = curFile.Name[:len(curFile.Name)-1]
}
if curFile.OldName == "" {
curFile.OldName = curFile.Name
}
} else {
curFile.Name = curFile.OldName
}
curFile.IsAmbiguous = false
}
// Otherwise do nothing with this line, but now switch to parsing hunks
lineBytes, isFragment, err := parseHunks(curFile, maxLines, maxLineCharacters, input)
diff.TotalAddition += curFile.Addition
diff.TotalDeletion += curFile.Deletion
Expand Down Expand Up @@ -1056,13 +1103,33 @@ func createDiffFile(diff *Diff, line string) *DiffFile {

rd := strings.NewReader(line[len(cmdDiffHead):] + " ")
curFile.Type = DiffFileChange
curFile.OldName = readFileName(rd)
curFile.Name = readFileName(rd)
oldNameAmbiguity := false
newNameAmbiguity := false

curFile.OldName, oldNameAmbiguity = readFileName(rd)
curFile.Name, newNameAmbiguity = readFileName(rd)
if oldNameAmbiguity && newNameAmbiguity {
curFile.IsAmbiguous = true
// OK we should bet that the oldName and the newName are the same if they can be made to be same
// So we need to start again ...
if (len(line)-len(cmdDiffHead)-1)%2 == 0 {
// diff --git a/b b/b b/b b/b b/b b/b
//
midpoint := (len(line) + len(cmdDiffHead) - 1) / 2
new, old := line[len(cmdDiffHead):midpoint], line[midpoint+1:]
if len(new) > 2 && len(old) > 2 && new[2:] == old[2:] {
curFile.OldName = old[2:]
curFile.Name = old[2:]
}
}
}

curFile.IsRenamed = curFile.Name != curFile.OldName
return curFile
}

func readFileName(rd *strings.Reader) string {
func readFileName(rd *strings.Reader) (string, bool) {
ambiguity := false
var name string
char, _ := rd.ReadByte()
_ = rd.UnreadByte()
Expand All @@ -1072,9 +1139,24 @@ func readFileName(rd *strings.Reader) string {
name = name[1:]
}
} else {
// This technique is potentially ambiguous it may not be possible to uniquely identify the filenames from the diff line alone
ambiguity = true
fmt.Fscanf(rd, "%s ", &name)
char, _ := rd.ReadByte()
_ = rd.UnreadByte()
for !(char == 0 || char == '"' || char == 'b') {
var suffix string
fmt.Fscanf(rd, "%s ", &suffix)
name += " " + suffix
char, _ = rd.ReadByte()
_ = rd.UnreadByte()
}
}
if len(name) < 2 {
log.Error("Unable to determine name from reader: %v", rd)
return "", true
}
return name[2:]
return name[2:], ambiguity
}

// GetDiffRange builds a Diff between two commits of a repository.
Expand Down Expand Up @@ -1191,6 +1273,7 @@ func CommentAsDiff(c *models.Comment) (*Diff, error) {
diff, err := ParsePatch(setting.Git.MaxGitDiffLines,
setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(c.Patch))
if err != nil {
log.Error("Unable to parse patch: %v", err)
return nil, err
}
if len(diff.Files) == 0 {
Expand Down
76 changes: 68 additions & 8 deletions services/gitdiff/gitdiff_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,66 @@ rename to a b/a a/file b/b file
oldFilename: "a b/file b/a a/file",
filename: "a b/a a/file b/b file",
},
{
name: "ambiguous deleted",
gitdiff: `diff --git a/b b/b b/b b/b
deleted file mode 100644
index 92e798b..0000000
--- a/b b/b` + "\t" + `
+++ /dev/null
@@ -1 +0,0 @@
-b b/b
`,
oldFilename: "b b/b",
filename: "b b/b",
addition: 0,
deletion: 1,
},
{
name: "ambiguous addition",
gitdiff: `diff --git a/b b/b b/b b/b
new file mode 100644
index 0000000..92e798b
--- /dev/null
+++ b/b b/b` + "\t" + `
@@ -0,0 +1 @@
+b b/b
`,
oldFilename: "b b/b",
filename: "b b/b",
addition: 1,
deletion: 0,
},
{
name: "rename",
gitdiff: `diff --git a/b b/b b/b b/b b/b b/b
similarity index 100%
rename from b b/b b/b b/b b/b
rename to b
`,
oldFilename: "b b/b b/b b/b b/b",
filename: "b",
},
{
name: "ambiguous 1",
gitdiff: `diff --git a/b b/b b/b b/b b/b b/b
similarity index 100%
rename from b b/b b/b b/b b/b
rename to b
`,
oldFilename: "b b/b b/b b/b b/b",
filename: "b",
},
{
name: "ambiguous 2",
gitdiff: `diff --git a/b b/b b/b b/b b/b b/b
similarity index 100%
rename from b b/b b/b b/b
rename to b b/b
`,
oldFilename: "b b/b b/b b/b",
filename: "b b/b",
},
{
name: "minuses-and-pluses",
gitdiff: `diff --git a/minuses-and-pluses b/minuses-and-pluses
Expand Down Expand Up @@ -235,32 +295,32 @@ index 6961180..9ba1a00 100644
t.Run(testcase.name, func(t *testing.T) {
got, err := ParsePatch(setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(testcase.gitdiff))
if (err != nil) != testcase.wantErr {
t.Errorf("ParsePatch() error = %v, wantErr %v", err, testcase.wantErr)
t.Errorf("ParsePatch(%q) error = %v, wantErr %v", testcase.name, err, testcase.wantErr)
return
}
gotMarshaled, _ := json.MarshalIndent(got, " ", " ")
if got.NumFiles != 1 {
t.Errorf("ParsePath() did not receive 1 file:\n%s", string(gotMarshaled))
t.Errorf("ParsePath(%q) did not receive 1 file:\n%s", testcase.name, string(gotMarshaled))
return
}
if got.TotalAddition != testcase.addition {
t.Errorf("ParsePath() does not have correct totalAddition %d, wanted %d", got.TotalAddition, testcase.addition)
t.Errorf("ParsePath(%q) does not have correct totalAddition %d, wanted %d", testcase.name, got.TotalAddition, testcase.addition)
}
if got.TotalDeletion != testcase.deletion {
t.Errorf("ParsePath() did not have correct totalDeletion %d, wanted %d", got.TotalDeletion, testcase.deletion)
t.Errorf("ParsePath(%q) did not have correct totalDeletion %d, wanted %d", testcase.name, got.TotalDeletion, testcase.deletion)
}
file := got.Files[0]
if file.Addition != testcase.addition {
t.Errorf("ParsePath() does not have correct file addition %d, wanted %d", file.Addition, testcase.addition)
t.Errorf("ParsePath(%q) does not have correct file addition %d, wanted %d", testcase.name, file.Addition, testcase.addition)
}
if file.Deletion != testcase.deletion {
t.Errorf("ParsePath() did not have correct file deletion %d, wanted %d", file.Deletion, testcase.deletion)
t.Errorf("ParsePath(%q) did not have correct file deletion %d, wanted %d", testcase.name, file.Deletion, testcase.deletion)
}
if file.OldName != testcase.oldFilename {
t.Errorf("ParsePath() did not have correct OldName %s, wanted %s", file.OldName, testcase.oldFilename)
t.Errorf("ParsePath(%q) did not have correct OldName %q, wanted %q", testcase.name, file.OldName, testcase.oldFilename)
}
if file.Name != testcase.filename {
t.Errorf("ParsePath() did not have correct Name %s, wanted %s", file.Name, testcase.filename)
t.Errorf("ParsePath(%q) did not have correct Name %q, wanted %q", testcase.name, file.Name, testcase.filename)
}
})
}
Expand Down

0 comments on commit bc2d0f3

Please sign in to comment.