From edfebe65b14c167d5d14169334c4ba37dbebb700 Mon Sep 17 00:00:00 2001 From: zeripath Date: Wed, 14 Oct 2020 05:49:33 +0100 Subject: [PATCH] Finally fix diff names (#13136) * Finally fix diff names #12771 attempted to fix diff by avoiding the git diff line as it is possible to have an ambiguous line here. #12254 attempted to fix diff by assuming that names would quoted if they needed to be and if one was quoted then both would be. Both of these were wrong. I have now discovered `--src-prefix` and `--dst-prefix` which means that we can set this in such a way to force the git diff to always be unambiguous. Therefore this PR rollsback most of the changes in #12771 and uses these options to fix this. Signed-off-by: Andrew Thornton * Update services/gitdiff/gitdiff.go * Update services/gitdiff/gitdiff.go * Update modules/repofiles/temp_repo.go * fix test Signed-off-by: Andrew Thornton Co-authored-by: Lauris BH --- modules/repofiles/temp_repo.go | 2 +- services/gitdiff/gitdiff.go | 164 +++++++++++-------------------- services/gitdiff/gitdiff_test.go | 39 ++++---- 3 files changed, 81 insertions(+), 124 deletions(-) diff --git a/modules/repofiles/temp_repo.go b/modules/repofiles/temp_repo.go index e0d6c9fcb6..49a7ddbf19 100644 --- a/modules/repofiles/temp_repo.go +++ b/modules/repofiles/temp_repo.go @@ -292,7 +292,7 @@ func (t *TemporaryUploadRepository) DiffIndex() (*gitdiff.Diff, error) { var diff *gitdiff.Diff var finalErr error - if err := git.NewCommand("diff-index", "--cached", "-p", "HEAD"). + if err := git.NewCommand("diff-index", "--src-prefix=\\a/", "--dst-prefix=\\b/", "--cached", "-p", "HEAD"). RunInDirTimeoutEnvFullPipelineFunc(nil, 30*time.Second, t.basePath, stdoutWriter, stderr, nil, func(ctx context.Context, cancel context.CancelFunc) error { _ = stdoutWriter.Close() diff, finalErr = gitdiff.ParsePatch(setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, stdoutReader) diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index de83b518a1..1882b48ed4 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -483,46 +483,7 @@ 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 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:] - } - continue - } else if strings.HasPrefix(line, "+++ ") { - if line[4] == '"' { - 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:] - } - 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 - } - - if strings.HasPrefix(line, "+++") || strings.HasPrefix(line, "---") || len(line) == 0 { + if strings.HasPrefix(line, "+++ ") || strings.HasPrefix(line, "--- ") || len(line) == 0 { continue } @@ -610,10 +571,42 @@ 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) + if a[0] == '\\' { + a = a[1:] + } + } else { + fmt.Fscanf(rd, "%s ", &a) + } + char, _ = rd.ReadByte() + _ = rd.UnreadByte() + if char == '"' { + fmt.Fscanf(rd, "%q", &b) + if b[0] == '\\' { + b = b[1:] + } + } else { + fmt.Fscanf(rd, "%s", &b) + } + a = a[2:] + b = b[2:] + curFile = &DiffFile{ - Index: len(diff.Files) + 1, - Type: DiffFileChange, - Sections: make([]*DiffSection, 0, 10), + Name: b, + OldName: a, + Index: len(diff.Files) + 1, + Type: DiffFileChange, + Sections: make([]*DiffSection, 0, 10), + IsRenamed: a != b, } diff.Files = append(diff.Files, curFile) curFileLinesCount = 0 @@ -622,7 +615,6 @@ 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 { @@ -633,67 +625,29 @@ func ParsePatch(maxLines, maxLineCharacters, maxFiles int, reader io.Reader) (*D } } - 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:] - 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 - case strings.HasPrefix(line, "copy from "): - if line[10] == '"' { - 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 - break loop - default: - if strings.HasSuffix(line, " 160000\n") { - curFile.IsSubmodule = true - } else { - break loop - } + switch { + case strings.HasPrefix(line, "copy from "): + curFile.IsRenamed = true + curFile.Type = DiffFileCopy + case strings.HasPrefix(line, "copy to "): + curFile.IsRenamed = true + curFile.Type = DiffFileCopy + 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 } + break } } } @@ -762,7 +716,7 @@ func GetDiffRangeWithWhitespaceBehavior(repoPath, beforeCommitID, afterCommitID parentCommit, _ := commit.Parent(0) actualBeforeCommitID = parentCommit.ID.String() } - diffArgs := []string{"diff", "-M"} + diffArgs := []string{"diff", "--src-prefix=\\a/", "--dst-prefix=\\b/", "-M"} if len(whitespaceBehavior) != 0 { diffArgs = append(diffArgs, whitespaceBehavior) } diff --git a/services/gitdiff/gitdiff_test.go b/services/gitdiff/gitdiff_test.go index ad1d186e73..b90ad657f6 100644 --- a/services/gitdiff/gitdiff_test.go +++ b/services/gitdiff/gitdiff_test.go @@ -90,9 +90,9 @@ func TestParsePatch_singlefile(t *testing.T) { tests := []testcase{ { name: "readme.md2readme.md", - gitdiff: `diff --git "a/README.md" "b/README.md" ---- a/README.md -+++ b/README.md + gitdiff: `diff --git "\\a/README.md" "\\b/README.md" +--- "\\a/README.md" ++++ "\\b/README.md" @@ -1,3 +1,6 @@ # gitea-github-migrator + @@ -102,9 +102,10 @@ func TestParsePatch_singlefile(t *testing.T) { + cut off + cut off `, - addition: 4, - deletion: 1, - filename: "README.md", + addition: 4, + deletion: 1, + filename: "README.md", + oldFilename: "README.md", }, { name: "A \\ B", @@ -119,16 +120,17 @@ func TestParsePatch_singlefile(t *testing.T) { Docker Pulls + cut off + cut off`, - addition: 4, - deletion: 1, - filename: "A \\ B", + addition: 4, + deletion: 1, + filename: "A \\ B", + oldFilename: "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 + 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 +--- "\\a/a b/file b/a a/file" ++++ "\\b/a b/file b/a a/file" @@ -1,3 +1,2 @@ Create a weird file. @@ -141,10 +143,10 @@ index d2186f1..f5c8ed2 100644 }, { name: "delete file with blanks", - gitdiff: `diff --git a/file with blanks b/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 +--- "\\a/file with blanks" +++ /dev/null @@ -1,5 +0,0 @@ -a blank file @@ -153,9 +155,10 @@ index 898651a..0000000 - -the 5th line is the last `, - addition: 0, - deletion: 5, - filename: "file with blanks", + addition: 0, + deletion: 5, + filename: "file with blanks", + oldFilename: "file with blanks", }, { name: "rename a—as", @@ -171,7 +174,7 @@ rename to "a\342\200\224as" }, { name: "rename with spaces", - gitdiff: `diff --git a/a b/file b/a a/file b/a b/a a/file b/b file + 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