Skip to content

Commit 5955d56

Browse files
zeripathzeripath
committed
Fix yet another bug with diff file names (#12771)
Backport #12771 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 <[email protected]>
1 parent 7a25441 commit 5955d56

File tree

3 files changed

+246
-48
lines changed

3 files changed

+246
-48
lines changed

‎modules/templates/helper.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -619,7 +619,7 @@ func ActionContent2Commits(act Actioner) *repository.PushCommits {
619619
// DiffTypeToStr returns diff type name
620620
func DiffTypeToStr(diffType int) string {
621621
diffTypes := map[int]string{
622-
1: "add", 2: "modify", 3: "del", 4: "rename",
622+
1: "add", 2: "modify", 3: "del", 4: "rename", 5: "copy",
623623
}
624624
return diffTypes[diffType]
625625
}

‎services/gitdiff/gitdiff.go

+105-46
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ const (
5353
DiffFileChange
5454
DiffFileDel
5555
DiffFileRename
56+
DiffFileCopy
5657
)
5758

5859
// DiffLineExpandDirection represents the DiffLineSection expand direction
@@ -447,7 +448,46 @@ func ParsePatch(maxLines, maxLineCharacters, maxFiles int, reader io.Reader) (*D
447448
}
448449
line := linebuf.String()
449450

450-
if strings.HasPrefix(line, "+++ ") || strings.HasPrefix(line, "--- ") || len(line) == 0 {
451+
if strings.HasPrefix(line, "--- ") {
452+
if line[4] == '"' {
453+
fmt.Sscanf(line[4:], "%q", &curFile.OldName)
454+
} else {
455+
curFile.OldName = line[4:]
456+
if strings.Contains(curFile.OldName, " ") {
457+
// Git adds a terminal \t if there is a space in the name
458+
curFile.OldName = curFile.OldName[:len(curFile.OldName)-1]
459+
}
460+
}
461+
if curFile.OldName[0:2] == "a/" {
462+
curFile.OldName = curFile.OldName[2:]
463+
}
464+
continue
465+
} else if strings.HasPrefix(line, "+++ ") {
466+
if line[4] == '"' {
467+
fmt.Sscanf(line[4:], "%q", &curFile.Name)
468+
} else {
469+
curFile.Name = line[4:]
470+
if strings.Contains(curFile.Name, " ") {
471+
// Git adds a terminal \t if there is a space in the name
472+
curFile.Name = curFile.Name[:len(curFile.Name)-1]
473+
}
474+
}
475+
if curFile.Name[0:2] == "b/" {
476+
curFile.Name = curFile.Name[2:]
477+
}
478+
curFile.IsRenamed = (curFile.Name != curFile.OldName) && !(curFile.IsCreated || curFile.IsDeleted)
479+
if curFile.IsDeleted {
480+
curFile.Name = curFile.OldName
481+
curFile.OldName = ""
482+
} else if curFile.IsCreated {
483+
curFile.OldName = ""
484+
}
485+
continue
486+
} else if len(line) == 0 {
487+
continue
488+
}
489+
490+
if strings.HasPrefix(line, "+++") || strings.HasPrefix(line, "---") || len(line) == 0 {
451491
continue
452492
}
453493

@@ -531,36 +571,10 @@ func ParsePatch(maxLines, maxLineCharacters, maxFiles int, reader io.Reader) (*D
531571
break
532572
}
533573

534-
// Note: In case file name is surrounded by double quotes (it happens only in git-shell).
535-
// e.g. diff --git "a/xxx" "b/xxx"
536-
var a string
537-
var b string
538-
539-
rd := strings.NewReader(line[len(cmdDiffHead):])
540-
char, _ := rd.ReadByte()
541-
_ = rd.UnreadByte()
542-
if char == '"' {
543-
fmt.Fscanf(rd, "%q ", &a)
544-
} else {
545-
fmt.Fscanf(rd, "%s ", &a)
546-
}
547-
char, _ = rd.ReadByte()
548-
_ = rd.UnreadByte()
549-
if char == '"' {
550-
fmt.Fscanf(rd, "%q", &b)
551-
} else {
552-
fmt.Fscanf(rd, "%s", &b)
553-
}
554-
a = a[2:]
555-
b = b[2:]
556-
557574
curFile = &DiffFile{
558-
Name: b,
559-
OldName: a,
560-
Index: len(diff.Files) + 1,
561-
Type: DiffFileChange,
562-
Sections: make([]*DiffSection, 0, 10),
563-
IsRenamed: a != b,
575+
Index: len(diff.Files) + 1,
576+
Type: DiffFileChange,
577+
Sections: make([]*DiffSection, 0, 10),
564578
}
565579
diff.Files = append(diff.Files, curFile)
566580
curFileLinesCount = 0
@@ -569,6 +583,7 @@ func ParsePatch(maxLines, maxLineCharacters, maxFiles int, reader io.Reader) (*D
569583
curFileLFSPrefix = false
570584

571585
// Check file diff type and is submodule.
586+
loop:
572587
for {
573588
line, err := input.ReadString('\n')
574589
if err != nil {
@@ -579,23 +594,67 @@ func ParsePatch(maxLines, maxLineCharacters, maxFiles int, reader io.Reader) (*D
579594
}
580595
}
581596

582-
switch {
583-
case strings.HasPrefix(line, "new file"):
584-
curFile.Type = DiffFileAdd
585-
curFile.IsCreated = true
586-
case strings.HasPrefix(line, "deleted"):
587-
curFile.Type = DiffFileDel
588-
curFile.IsDeleted = true
589-
case strings.HasPrefix(line, "index"):
590-
curFile.Type = DiffFileChange
591-
case strings.HasPrefix(line, "similarity index 100%"):
592-
curFile.Type = DiffFileRename
593-
}
594-
if curFile.Type > 0 {
595-
if strings.HasSuffix(line, " 160000\n") {
596-
curFile.IsSubmodule = true
597+
if curFile.Type != DiffFileRename {
598+
switch {
599+
case strings.HasPrefix(line, "new file"):
600+
curFile.Type = DiffFileAdd
601+
curFile.IsCreated = true
602+
case strings.HasPrefix(line, "deleted"):
603+
curFile.Type = DiffFileDel
604+
curFile.IsDeleted = true
605+
case strings.HasPrefix(line, "index"):
606+
curFile.Type = DiffFileChange
607+
case strings.HasPrefix(line, "similarity index 100%"):
608+
curFile.Type = DiffFileRename
609+
}
610+
if curFile.Type > 0 && curFile.Type != DiffFileRename {
611+
if strings.HasSuffix(line, " 160000\n") {
612+
curFile.IsSubmodule = true
613+
}
614+
break
615+
}
616+
} else {
617+
switch {
618+
case strings.HasPrefix(line, "rename from "):
619+
if line[12] == '"' {
620+
fmt.Sscanf(line[12:], "%q", &curFile.OldName)
621+
} else {
622+
curFile.OldName = line[12:]
623+
curFile.OldName = curFile.OldName[:len(curFile.OldName)-1]
624+
}
625+
case strings.HasPrefix(line, "rename to "):
626+
if line[10] == '"' {
627+
fmt.Sscanf(line[10:], "%q", &curFile.Name)
628+
} else {
629+
curFile.Name = line[10:]
630+
curFile.Name = curFile.Name[:len(curFile.Name)-1]
631+
}
632+
curFile.IsRenamed = true
633+
break loop
634+
case strings.HasPrefix(line, "copy from "):
635+
if line[10] == '"' {
636+
fmt.Sscanf(line[10:], "%q", &curFile.OldName)
637+
} else {
638+
curFile.OldName = line[10:]
639+
curFile.OldName = curFile.OldName[:len(curFile.OldName)-1]
640+
}
641+
case strings.HasPrefix(line, "copy to "):
642+
if line[8] == '"' {
643+
fmt.Sscanf(line[8:], "%q", &curFile.Name)
644+
} else {
645+
curFile.Name = line[8:]
646+
curFile.Name = curFile.Name[:len(curFile.Name)-1]
647+
}
648+
curFile.IsRenamed = true
649+
curFile.Type = DiffFileCopy
650+
break loop
651+
default:
652+
if strings.HasSuffix(line, " 160000\n") {
653+
curFile.IsSubmodule = true
654+
} else {
655+
break loop
656+
}
597657
}
598-
break
599658
}
600659
}
601660
}

‎services/gitdiff/gitdiff_test.go

+140-1
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
package gitdiff
77

88
import (
9+
"encoding/json"
910
"fmt"
1011
"html/template"
1112
"strings"
@@ -41,7 +42,145 @@ func TestDiffToHTML(t *testing.T) {
4142
}, DiffLineDel))
4243
}
4344

44-
func TestParsePatch(t *testing.T) {
45+
func TestParsePatch_singlefile(t *testing.T) {
46+
type testcase struct {
47+
name string
48+
gitdiff string
49+
wantErr bool
50+
addition int
51+
deletion int
52+
oldFilename string
53+
filename string
54+
}
55+
56+
tests := []testcase{
57+
{
58+
name: "readme.md2readme.md",
59+
gitdiff: `diff --git "a/README.md" "b/README.md"
60+
--- a/README.md
61+
+++ b/README.md
62+
@@ -1,3 +1,6 @@
63+
# gitea-github-migrator
64+
+
65+
+ Build Status
66+
- Latest Release
67+
Docker Pulls
68+
+ cut off
69+
+ cut off
70+
`,
71+
addition: 4,
72+
deletion: 1,
73+
filename: "README.md",
74+
},
75+
{
76+
name: "A \\ B",
77+
gitdiff: `diff --git "a/A \\ B" "b/A \\ B"
78+
--- "a/A \\ B"
79+
+++ "b/A \\ B"
80+
@@ -1,3 +1,6 @@
81+
# gitea-github-migrator
82+
+
83+
+ Build Status
84+
- Latest Release
85+
Docker Pulls
86+
+ cut off
87+
+ cut off`,
88+
addition: 4,
89+
deletion: 1,
90+
filename: "A \\ B",
91+
},
92+
{
93+
name: "really weird filename",
94+
gitdiff: `diff --git a/a b/file b/a a/file b/a b/file b/a a/file
95+
index d2186f1..f5c8ed2 100644
96+
--- a/a b/file b/a a/file
97+
+++ b/a b/file b/a a/file
98+
@@ -1,3 +1,2 @@
99+
Create a weird file.
100+
101+
-and what does diff do here?
102+
\ No newline at end of file`,
103+
addition: 0,
104+
deletion: 1,
105+
filename: "a b/file b/a a/file",
106+
oldFilename: "a b/file b/a a/file",
107+
},
108+
{
109+
name: "delete file with blanks",
110+
gitdiff: `diff --git a/file with blanks b/file with blanks
111+
deleted file mode 100644
112+
index 898651a..0000000
113+
--- a/file with blanks
114+
+++ /dev/null
115+
@@ -1,5 +0,0 @@
116+
-a blank file
117+
-
118+
-has a couple o line
119+
-
120+
-the 5th line is the last
121+
`,
122+
addition: 0,
123+
deletion: 5,
124+
filename: "file with blanks",
125+
},
126+
{
127+
name: "rename a—as",
128+
gitdiff: `diff --git "a/\360\243\220\265b\342\200\240vs" "b/a\342\200\224as"
129+
similarity index 100%
130+
rename from "\360\243\220\265b\342\200\240vs"
131+
rename to "a\342\200\224as"
132+
`,
133+
addition: 0,
134+
deletion: 0,
135+
oldFilename: "𣐵b†vs",
136+
filename: "a—as",
137+
},
138+
{
139+
name: "rename with spaces",
140+
gitdiff: `diff --git a/a b/file b/a a/file b/a b/a a/file b/b file
141+
similarity index 100%
142+
rename from a b/file b/a a/file
143+
rename to a b/a a/file b/b file
144+
`,
145+
oldFilename: "a b/file b/a a/file",
146+
filename: "a b/a a/file b/b file",
147+
},
148+
}
149+
150+
for _, testcase := range tests {
151+
t.Run(testcase.name, func(t *testing.T) {
152+
got, err := ParsePatch(setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(testcase.gitdiff))
153+
if (err != nil) != testcase.wantErr {
154+
t.Errorf("ParsePatch() error = %v, wantErr %v", err, testcase.wantErr)
155+
return
156+
}
157+
gotMarshaled, _ := json.MarshalIndent(got, " ", " ")
158+
if got.NumFiles() != 1 {
159+
t.Errorf("ParsePath() did not receive 1 file:\n%s", string(gotMarshaled))
160+
return
161+
}
162+
if got.TotalAddition != testcase.addition {
163+
t.Errorf("ParsePath() does not have correct totalAddition %d, wanted %d", got.TotalAddition, testcase.addition)
164+
}
165+
if got.TotalDeletion != testcase.deletion {
166+
t.Errorf("ParsePath() did not have correct totalDeletion %d, wanted %d", got.TotalDeletion, testcase.deletion)
167+
}
168+
file := got.Files[0]
169+
if file.Addition != testcase.addition {
170+
t.Errorf("ParsePath() does not have correct file addition %d, wanted %d", file.Addition, testcase.addition)
171+
}
172+
if file.Deletion != testcase.deletion {
173+
t.Errorf("ParsePath() did not have correct file deletion %d, wanted %d", file.Deletion, testcase.deletion)
174+
}
175+
if file.OldName != testcase.oldFilename {
176+
t.Errorf("ParsePath() did not have correct OldName %s, wanted %s", file.OldName, testcase.oldFilename)
177+
}
178+
if file.Name != testcase.filename {
179+
t.Errorf("ParsePath() did not have correct Name %s, wanted %s", file.Name, testcase.filename)
180+
}
181+
})
182+
}
183+
45184
var diff = `diff --git "a/README.md" "b/README.md"
46185
--- a/README.md
47186
+++ b/README.md

0 commit comments

Comments
 (0)