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 <art27@cantab.net>

* Update services/gitdiff/gitdiff.go

* Update services/gitdiff/gitdiff.go

* Update modules/repofiles/temp_repo.go

* fix test

Signed-off-by: Andrew Thornton <art27@cantab.net>

Co-authored-by: Lauris BH <lauris@nix.lv>
release/v1.15
zeripath 2020-10-14 05:49:33 +01:00 committed by GitHub
parent aa73b7b7e5
commit edfebe65b1
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 81 additions and 124 deletions

View File

@ -292,7 +292,7 @@ func (t *TemporaryUploadRepository) DiffIndex() (*gitdiff.Diff, error) {
var diff *gitdiff.Diff var diff *gitdiff.Diff
var finalErr error 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 { RunInDirTimeoutEnvFullPipelineFunc(nil, 30*time.Second, t.basePath, stdoutWriter, stderr, nil, func(ctx context.Context, cancel context.CancelFunc) error {
_ = stdoutWriter.Close() _ = stdoutWriter.Close()
diff, finalErr = gitdiff.ParsePatch(setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, stdoutReader) diff, finalErr = gitdiff.ParsePatch(setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, stdoutReader)

View File

@ -483,46 +483,7 @@ func ParsePatch(maxLines, maxLineCharacters, maxFiles int, reader io.Reader) (*D
} }
line := linebuf.String() line := linebuf.String()
if strings.HasPrefix(line, "--- ") { if strings.HasPrefix(line, "+++ ") || strings.HasPrefix(line, "--- ") || len(line) == 0 {
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 {
continue continue
} }
@ -610,10 +571,42 @@ func ParsePatch(maxLines, maxLineCharacters, maxFiles int, reader io.Reader) (*D
break 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{ curFile = &DiffFile{
Index: len(diff.Files) + 1, Name: b,
Type: DiffFileChange, OldName: a,
Sections: make([]*DiffSection, 0, 10), Index: len(diff.Files) + 1,
Type: DiffFileChange,
Sections: make([]*DiffSection, 0, 10),
IsRenamed: a != b,
} }
diff.Files = append(diff.Files, curFile) diff.Files = append(diff.Files, curFile)
curFileLinesCount = 0 curFileLinesCount = 0
@ -622,7 +615,6 @@ func ParsePatch(maxLines, maxLineCharacters, maxFiles int, reader io.Reader) (*D
curFileLFSPrefix = false curFileLFSPrefix = false
// Check file diff type and is submodule. // Check file diff type and is submodule.
loop:
for { for {
line, err := input.ReadString('\n') line, err := input.ReadString('\n')
if err != nil { if err != nil {
@ -633,67 +625,29 @@ func ParsePatch(maxLines, maxLineCharacters, maxFiles int, reader io.Reader) (*D
} }
} }
if curFile.Type != DiffFileRename { switch {
switch { case strings.HasPrefix(line, "copy from "):
case strings.HasPrefix(line, "new file"): curFile.IsRenamed = true
curFile.Type = DiffFileAdd curFile.Type = DiffFileCopy
curFile.IsCreated = true case strings.HasPrefix(line, "copy to "):
case strings.HasPrefix(line, "deleted"): curFile.IsRenamed = true
curFile.Type = DiffFileDel curFile.Type = DiffFileCopy
curFile.IsDeleted = true case strings.HasPrefix(line, "new file"):
case strings.HasPrefix(line, "index"): curFile.Type = DiffFileAdd
curFile.Type = DiffFileChange curFile.IsCreated = true
case strings.HasPrefix(line, "similarity index 100%"): case strings.HasPrefix(line, "deleted"):
curFile.Type = DiffFileRename curFile.Type = DiffFileDel
} curFile.IsDeleted = true
if curFile.Type > 0 && curFile.Type != DiffFileRename { case strings.HasPrefix(line, "index"):
if strings.HasSuffix(line, " 160000\n") { curFile.Type = DiffFileChange
curFile.IsSubmodule = true case strings.HasPrefix(line, "similarity index 100%"):
} curFile.Type = DiffFileRename
break }
} if curFile.Type > 0 {
} else { if strings.HasSuffix(line, " 160000\n") {
switch { curFile.IsSubmodule = true
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
}
} }
break
} }
} }
} }
@ -762,7 +716,7 @@ func GetDiffRangeWithWhitespaceBehavior(repoPath, beforeCommitID, afterCommitID
parentCommit, _ := commit.Parent(0) parentCommit, _ := commit.Parent(0)
actualBeforeCommitID = parentCommit.ID.String() actualBeforeCommitID = parentCommit.ID.String()
} }
diffArgs := []string{"diff", "-M"} diffArgs := []string{"diff", "--src-prefix=\\a/", "--dst-prefix=\\b/", "-M"}
if len(whitespaceBehavior) != 0 { if len(whitespaceBehavior) != 0 {
diffArgs = append(diffArgs, whitespaceBehavior) diffArgs = append(diffArgs, whitespaceBehavior)
} }

View File

@ -90,9 +90,9 @@ func TestParsePatch_singlefile(t *testing.T) {
tests := []testcase{ tests := []testcase{
{ {
name: "readme.md2readme.md", name: "readme.md2readme.md",
gitdiff: `diff --git "a/README.md" "b/README.md" gitdiff: `diff --git "\\a/README.md" "\\b/README.md"
--- a/README.md --- "\\a/README.md"
+++ b/README.md +++ "\\b/README.md"
@@ -1,3 +1,6 @@ @@ -1,3 +1,6 @@
# gitea-github-migrator # gitea-github-migrator
+ +
@ -102,9 +102,10 @@ func TestParsePatch_singlefile(t *testing.T) {
+ cut off + cut off
+ cut off + cut off
`, `,
addition: 4, addition: 4,
deletion: 1, deletion: 1,
filename: "README.md", filename: "README.md",
oldFilename: "README.md",
}, },
{ {
name: "A \\ B", name: "A \\ B",
@ -119,16 +120,17 @@ func TestParsePatch_singlefile(t *testing.T) {
Docker Pulls Docker Pulls
+ cut off + cut off
+ cut off`, + cut off`,
addition: 4, addition: 4,
deletion: 1, deletion: 1,
filename: "A \\ B", filename: "A \\ B",
oldFilename: "A \\ B",
}, },
{ {
name: "really weird filename", 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 index d2186f1..f5c8ed2 100644
--- a/a b/file b/a a/file --- "\\a/a b/file b/a a/file"
+++ b/a b/file b/a a/file +++ "\\b/a b/file b/a a/file"
@@ -1,3 +1,2 @@ @@ -1,3 +1,2 @@
Create a weird file. Create a weird file.
@ -141,10 +143,10 @@ index d2186f1..f5c8ed2 100644
}, },
{ {
name: "delete file with blanks", 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 deleted file mode 100644
index 898651a..0000000 index 898651a..0000000
--- a/file with blanks --- "\\a/file with blanks"
+++ /dev/null +++ /dev/null
@@ -1,5 +0,0 @@ @@ -1,5 +0,0 @@
-a blank file -a blank file
@ -153,9 +155,10 @@ index 898651a..0000000
- -
-the 5th line is the last -the 5th line is the last
`, `,
addition: 0, addition: 0,
deletion: 5, deletion: 5,
filename: "file with blanks", filename: "file with blanks",
oldFilename: "file with blanks",
}, },
{ {
name: "rename a—as", name: "rename a—as",
@ -171,7 +174,7 @@ rename to "a\342\200\224as"
}, },
{ {
name: "rename with spaces", 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% similarity index 100%
rename from a b/file b/a a/file rename from a b/file b/a a/file
rename to a b/a a/file b/b file rename to a b/a a/file b/b file