From 93563a59905e1168b996997c4c7594bb17d46820 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 8 Sep 2020 21:51:38 +0100 Subject: [PATCH 1/3] Fix yet another bug with diff file names Following further testing it has become apparent that the diff line cannot be used to determine filenames for diffs with any sort of predictability the answer therefore is to use the other lines that are provided with a diff Fix #12768 Signed-off-by: Andrew Thornton --- modules/templates/helper.go | 2 +- services/gitdiff/gitdiff.go | 131 +++++++++++++++++++++++------------- 2 files changed, 87 insertions(+), 46 deletions(-) diff --git a/modules/templates/helper.go b/modules/templates/helper.go index c5526b3dea144..9037af8991986 100644 --- a/modules/templates/helper.go +++ b/modules/templates/helper.go @@ -694,7 +694,7 @@ func ActionContent2Commits(act Actioner) *repository.PushCommits { // DiffTypeToStr returns diff type name func DiffTypeToStr(diffType int) string { diffTypes := map[int]string{ - 1: "add", 2: "modify", 3: "del", 4: "rename", + 1: "add", 2: "modify", 3: "del", 4: "rename", 5: "copy", } return diffTypes[diffType] } diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index 538f613b04588..d97a08537bb2e 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -53,6 +53,7 @@ const ( DiffFileChange DiffFileDel DiffFileRename + DiffFileCopy ) // DiffLineExpandDirection represents the DiffLineSection expand direction @@ -481,6 +482,31 @@ func ParsePatch(maxLines, maxLineCharacters, maxFiles int, reader io.Reader) (*D } line := linebuf.String() + if strings.HasPrefix(line, "--- ") { + if line[4] == '"' { + fmt.Sscanf(line[4:], "%q", &curFile.OldName) + } else { + curFile.OldName = line[4:] + } + if curFile.OldName[0:2] == "a/" { + curFile.OldName = curFile.OldName[2:] + } + continue + } else if strings.HasPrefix(line, "+++ ") { + if line[4] == '"' { + fmt.Sscanf(line[4:], "%q", &curFile.Name) + } else { + curFile.Name = line[4:] + } + if curFile.Name[0:2] == "b/" { + curFile.Name = curFile.Name[2:] + } + curFile.IsRenamed = (curFile.Name != curFile.OldName) && !(curFile.IsCreated || curFile.IsDeleted) + continue + } else if len(line) == 0 { + continue + } + if strings.HasPrefix(line, "+++ ") || strings.HasPrefix(line, "--- ") || len(line) == 0 { continue } @@ -569,36 +595,10 @@ func ParsePatch(maxLines, maxLineCharacters, maxFiles int, reader io.Reader) (*D break } - // Note: In case file name is surrounded by double quotes (it happens only in git-shell). - // e.g. diff --git "a/xxx" "b/xxx" - var a string - var b string - - rd := strings.NewReader(line[len(cmdDiffHead):]) - char, _ := rd.ReadByte() - _ = rd.UnreadByte() - if char == '"' { - fmt.Fscanf(rd, "%q ", &a) - } else { - fmt.Fscanf(rd, "%s ", &a) - } - char, _ = rd.ReadByte() - _ = rd.UnreadByte() - if char == '"' { - fmt.Fscanf(rd, "%q", &b) - } else { - fmt.Fscanf(rd, "%s", &b) - } - a = a[2:] - b = b[2:] - curFile = &DiffFile{ - Name: b, - OldName: a, - Index: len(diff.Files) + 1, - Type: DiffFileChange, - Sections: make([]*DiffSection, 0, 10), - IsRenamed: a != b, + Index: len(diff.Files) + 1, + Type: DiffFileChange, + Sections: make([]*DiffSection, 0, 10), } diff.Files = append(diff.Files, curFile) curFileLinesCount = 0 @@ -607,6 +607,7 @@ func ParsePatch(maxLines, maxLineCharacters, maxFiles int, reader io.Reader) (*D curFileLFSPrefix = false // Check file diff type and is submodule. + loop: for { line, err := input.ReadString('\n') if err != nil { @@ -617,23 +618,63 @@ func ParsePatch(maxLines, maxLineCharacters, maxFiles int, reader io.Reader) (*D } } - switch { - case strings.HasPrefix(line, "new file"): - curFile.Type = DiffFileAdd - curFile.IsCreated = true - case strings.HasPrefix(line, "deleted"): - curFile.Type = DiffFileDel - curFile.IsDeleted = true - case strings.HasPrefix(line, "index"): - curFile.Type = DiffFileChange - case strings.HasPrefix(line, "similarity index 100%"): - curFile.Type = DiffFileRename - } - if curFile.Type > 0 { - if strings.HasSuffix(line, " 160000\n") { - curFile.IsSubmodule = true + if curFile.Type != DiffFileRename { + switch { + case strings.HasPrefix(line, "new file"): + curFile.Type = DiffFileAdd + curFile.IsCreated = true + case strings.HasPrefix(line, "deleted"): + curFile.Type = DiffFileDel + curFile.IsDeleted = true + case strings.HasPrefix(line, "index"): + curFile.Type = DiffFileChange + case strings.HasPrefix(line, "similarity index 100%"): + curFile.Type = DiffFileRename + } + if curFile.Type > 0 && curFile.Type != DiffFileRename { + if strings.HasSuffix(line, " 160000\n") { + curFile.IsSubmodule = true + } + break + } + } else { + switch { + case strings.HasPrefix(line, "rename from "): + if line[12] == '"' { + fmt.Sscanf(line[12:], "%q", &curFile.OldName) + } else { + curFile.OldName = line[12:] + } + case strings.HasPrefix(line, "rename to "): + if line[10] == '"' { + fmt.Sscanf(line[10:], "%q", &curFile.Name) + } else { + curFile.Name = line[10:] + } + curFile.IsRenamed = true + break loop + case strings.HasPrefix(line, "copy from "): + if line[10] == '"' { + fmt.Sscanf(line[10:], "%q", &curFile.OldName) + } else { + curFile.OldName = line[10:] + } + case strings.HasPrefix(line, "copy to "): + if line[8] == '"' { + fmt.Sscanf(line[8:], "%q", &curFile.Name) + } else { + curFile.Name = line[8:] + } + curFile.IsRenamed = true + curFile.Type = DiffFileCopy + break loop + default: + if strings.HasSuffix(line, " 160000\n") { + curFile.IsSubmodule = true + } else { + break loop + } } - break } } } From 4f14229a71313f104c6c157464bf041ce57d2a7e Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Wed, 9 Sep 2020 08:56:44 +0100 Subject: [PATCH 2/3] Handle terminal tab if there is a space Signed-off-by: Andrew Thornton --- services/gitdiff/gitdiff.go | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index d97a08537bb2e..d182482d9c98c 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -487,6 +487,10 @@ func ParsePatch(maxLines, maxLineCharacters, maxFiles int, reader io.Reader) (*D fmt.Sscanf(line[4:], "%q", &curFile.OldName) } else { curFile.OldName = line[4:] + if strings.Contains(curFile.OldName, " ") { + // Git adds a terminal \t if there is a space in the name + curFile.OldName = curFile.OldName[:len(curFile.OldName)-1] + } } if curFile.OldName[0:2] == "a/" { curFile.OldName = curFile.OldName[2:] @@ -497,6 +501,10 @@ func ParsePatch(maxLines, maxLineCharacters, maxFiles int, reader io.Reader) (*D fmt.Sscanf(line[4:], "%q", &curFile.Name) } else { curFile.Name = line[4:] + if strings.Contains(curFile.Name, " ") { + // Git adds a terminal \t if there is a space in the name + curFile.Name = curFile.Name[:len(curFile.Name)-1] + } } if curFile.Name[0:2] == "b/" { curFile.Name = curFile.Name[2:] @@ -507,7 +515,7 @@ func ParsePatch(maxLines, maxLineCharacters, maxFiles int, reader io.Reader) (*D continue } - if strings.HasPrefix(line, "+++ ") || strings.HasPrefix(line, "--- ") || len(line) == 0 { + if strings.HasPrefix(line, "+++") || strings.HasPrefix(line, "---") || len(line) == 0 { continue } @@ -644,12 +652,14 @@ func ParsePatch(maxLines, maxLineCharacters, maxFiles int, reader io.Reader) (*D fmt.Sscanf(line[12:], "%q", &curFile.OldName) } else { curFile.OldName = line[12:] + curFile.OldName = curFile.OldName[:len(curFile.OldName)-1] } case strings.HasPrefix(line, "rename to "): if line[10] == '"' { fmt.Sscanf(line[10:], "%q", &curFile.Name) } else { curFile.Name = line[10:] + curFile.Name = curFile.Name[:len(curFile.Name)-1] } curFile.IsRenamed = true break loop @@ -658,12 +668,14 @@ func ParsePatch(maxLines, maxLineCharacters, maxFiles int, reader io.Reader) (*D fmt.Sscanf(line[10:], "%q", &curFile.OldName) } else { curFile.OldName = line[10:] + curFile.OldName = curFile.OldName[:len(curFile.OldName)-1] } case strings.HasPrefix(line, "copy to "): if line[8] == '"' { fmt.Sscanf(line[8:], "%q", &curFile.Name) } else { curFile.Name = line[8:] + curFile.Name = curFile.Name[:len(curFile.Name)-1] } curFile.IsRenamed = true curFile.Type = DiffFileCopy From cc4b79f01dd82ba487e52b80b35c80c95e73be5c Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Wed, 9 Sep 2020 10:04:17 +0100 Subject: [PATCH 3/3] Fix deletion and created names and improve testcases Signed-off-by: Andrew Thornton --- services/gitdiff/gitdiff.go | 6 ++ services/gitdiff/gitdiff_test.go | 145 ++++++++++++++++++++++++++++++- 2 files changed, 147 insertions(+), 4 deletions(-) diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index d182482d9c98c..4cea5dd9a0b18 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -510,6 +510,12 @@ func ParsePatch(maxLines, maxLineCharacters, maxFiles int, reader io.Reader) (*D curFile.Name = curFile.Name[2:] } curFile.IsRenamed = (curFile.Name != curFile.OldName) && !(curFile.IsCreated || curFile.IsDeleted) + if curFile.IsDeleted { + curFile.Name = curFile.OldName + curFile.OldName = "" + } else if curFile.IsCreated { + curFile.OldName = "" + } continue } else if len(line) == 0 { continue diff --git a/services/gitdiff/gitdiff_test.go b/services/gitdiff/gitdiff_test.go index 9826ece2dc5c9..ad1d186e732b0 100644 --- a/services/gitdiff/gitdiff_test.go +++ b/services/gitdiff/gitdiff_test.go @@ -6,6 +6,7 @@ package gitdiff import ( + "encoding/json" "fmt" "html/template" "strings" @@ -14,11 +15,9 @@ import ( "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/setting" - - "gopkg.in/ini.v1" - dmp "github.com/sergi/go-diff/diffmatchpatch" "github.com/stretchr/testify/assert" + "gopkg.in/ini.v1" ) func assertEqual(t *testing.T, s1 string, s2 template.HTML) { @@ -77,7 +76,145 @@ func TestDiffToHTML(t *testing.T) { }, DiffLineAdd)) } -func TestParsePatch(t *testing.T) { +func TestParsePatch_singlefile(t *testing.T) { + type testcase struct { + name string + gitdiff string + wantErr bool + addition int + deletion int + oldFilename string + filename string + } + + tests := []testcase{ + { + name: "readme.md2readme.md", + gitdiff: `diff --git "a/README.md" "b/README.md" +--- a/README.md ++++ b/README.md +@@ -1,3 +1,6 @@ + # gitea-github-migrator ++ ++ Build Status +- Latest Release + Docker Pulls ++ cut off ++ cut off +`, + addition: 4, + deletion: 1, + filename: "README.md", + }, + { + name: "A \\ B", + gitdiff: `diff --git "a/A \\ B" "b/A \\ B" +--- "a/A \\ B" ++++ "b/A \\ B" +@@ -1,3 +1,6 @@ + # gitea-github-migrator ++ ++ Build Status +- Latest Release + Docker Pulls ++ cut off ++ cut off`, + addition: 4, + deletion: 1, + filename: "A \\ B", + }, + { + name: "really weird filename", + gitdiff: `diff --git a/a b/file b/a a/file b/a b/file b/a a/file +index d2186f1..f5c8ed2 100644 +--- a/a b/file b/a a/file ++++ b/a b/file b/a a/file +@@ -1,3 +1,2 @@ + Create a weird file. + +-and what does diff do here? +\ No newline at end of file`, + addition: 0, + deletion: 1, + filename: "a b/file b/a a/file", + oldFilename: "a b/file b/a a/file", + }, + { + name: "delete file with blanks", + gitdiff: `diff --git a/file with blanks b/file with blanks +deleted file mode 100644 +index 898651a..0000000 +--- a/file with blanks ++++ /dev/null +@@ -1,5 +0,0 @@ +-a blank file +- +-has a couple o line +- +-the 5th line is the last +`, + addition: 0, + deletion: 5, + filename: "file with blanks", + }, + { + name: "rename a—as", + gitdiff: `diff --git "a/\360\243\220\265b\342\200\240vs" "b/a\342\200\224as" +similarity index 100% +rename from "\360\243\220\265b\342\200\240vs" +rename to "a\342\200\224as" +`, + addition: 0, + deletion: 0, + oldFilename: "𣐵b†vs", + filename: "a—as", + }, + { + name: "rename with spaces", + gitdiff: `diff --git a/a b/file b/a a/file b/a b/a a/file b/b file +similarity index 100% +rename from a b/file b/a a/file +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", + }, + } + + for _, testcase := range tests { + 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) + return + } + gotMarshaled, _ := json.MarshalIndent(got, " ", " ") + if got.NumFiles != 1 { + t.Errorf("ParsePath() did not receive 1 file:\n%s", string(gotMarshaled)) + return + } + if got.TotalAddition != testcase.addition { + t.Errorf("ParsePath() does not have correct totalAddition %d, wanted %d", got.TotalAddition, testcase.addition) + } + if got.TotalDeletion != testcase.deletion { + t.Errorf("ParsePath() did not have correct totalDeletion %d, wanted %d", 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) + } + if file.Deletion != testcase.deletion { + t.Errorf("ParsePath() did not have correct file deletion %d, wanted %d", file.Deletion, testcase.deletion) + } + if file.OldName != testcase.oldFilename { + t.Errorf("ParsePath() did not have correct OldName %s, wanted %s", file.OldName, testcase.oldFilename) + } + if file.Name != testcase.filename { + t.Errorf("ParsePath() did not have correct Name %s, wanted %s", file.Name, testcase.filename) + } + }) + } + var diff = `diff --git "a/README.md" "b/README.md" --- a/README.md +++ b/README.md