Finally fix diff names (#13136) (#13139)

Backport #13136

it is possible to have an ambiguous line here.

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>

modules/repofiles/temp_repo.go

@@ -278,7 +278,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)

services/gitdiff/gitdiff.go

@@ -448,46 +448,7 @@ func ParsePatch(maxLines, maxLineCharacters, maxFiles int, reader io.Reader) (*D
 		}
 		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
 		}
 
@@ -571,10 +532,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,
+				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)
 			curFileLinesCount = 0
@@ -583,7 +576,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 {
@@ -594,67 +586,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
 				}
 			}
 		}
@@ -723,7 +677,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)
 		}

services/gitdiff/gitdiff_test.go

@@ -56,9 +56,9 @@ func TestParsePatch_singlefile(t *testing.T) {
 	tests := []testcase{
 		{
 			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 @@
  # gitea-github-migrator
 +
@@ -68,9 +68,10 @@ func TestParsePatch_singlefile(t *testing.T) {
 + cut off
 + cut off
 `,
-			addition: 4,
+			addition:    4,
-			deletion: 1,
+			deletion:    1,
-			filename: "README.md",
+			filename:    "README.md",
+			oldFilename: "README.md",
 		},
 		{
 			name: "A \\ B",
@@ -85,16 +86,17 @@ func TestParsePatch_singlefile(t *testing.T) {
  Docker Pulls
 + 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",
-			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	
+--- "\\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 @@
  Create a weird file.
  
@@ -107,10 +109,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
@@ -119,9 +121,10 @@ index 898651a..0000000
 -
 -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",
@@ -137,7 +140,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