Skip to content

Commit 96969dd

Browse files
zeripathzeripath
authored
Fix yet another bug with diff file names (#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 1fbc50f commit 96969dd

File tree

3 files changed

+247
-51
lines changed

3 files changed

+247
-51
lines changed

‎modules/templates/helper.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -694,7 +694,7 @@ func ActionContent2Commits(act Actioner) *repository.PushCommits {
694694
// DiffTypeToStr returns diff type name
695695
func DiffTypeToStr(diffType int) string {
696696
diffTypes := map[int]string{
697-
1: "add", 2: "modify", 3: "del", 4: "rename",
697+
1: "add", 2: "modify", 3: "del", 4: "rename", 5: "copy",
698698
}
699699
return diffTypes[diffType]
700700
}

‎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
@@ -481,7 +482,46 @@ func ParsePatch(maxLines, maxLineCharacters, maxFiles int, reader io.Reader) (*D
481482
}
482483
line := linebuf.String()
483484

484-
if strings.HasPrefix(line, "+++ ") || strings.HasPrefix(line, "--- ") || len(line) == 0 {
485+
if strings.HasPrefix(line, "--- ") {
486+
if line[4] == '"' {
487+
fmt.Sscanf(line[4:], "%q", &curFile.OldName)
488+
} else {
489+
curFile.OldName = line[4:]
490+
if strings.Contains(curFile.OldName, " ") {
491+
// Git adds a terminal \t if there is a space in the name
492+
curFile.OldName = curFile.OldName[:len(curFile.OldName)-1]
493+
}
494+
}
495+
if curFile.OldName[0:2] == "a/" {
496+
curFile.OldName = curFile.OldName[2:]
497+
}
498+
continue
499+
} else if strings.HasPrefix(line, "+++ ") {
500+
if line[4] == '"' {
501+
fmt.Sscanf(line[4:], "%q", &curFile.Name)
502+
} else {
503+
curFile.Name = line[4:]
504+
if strings.Contains(curFile.Name, " ") {
505+
// Git adds a terminal \t if there is a space in the name
506+
curFile.Name = curFile.Name[:len(curFile.Name)-1]
507+
}
508+
}
509+
if curFile.Name[0:2] == "b/" {
510+
curFile.Name = curFile.Name[2:]
511+
}
512+
curFile.IsRenamed = (curFile.Name != curFile.OldName) && !(curFile.IsCreated || curFile.IsDeleted)
513+
if curFile.IsDeleted {
514+
curFile.Name = curFile.OldName
515+
curFile.OldName = ""
516+
} else if curFile.IsCreated {
517+
curFile.OldName = ""
518+
}
519+
continue
520+
} else if len(line) == 0 {
521+
continue
522+
}
523+
524+
if strings.HasPrefix(line, "+++") || strings.HasPrefix(line, "---") || len(line) == 0 {
485525
continue
486526
}
487527

@@ -569,36 +609,10 @@ func ParsePatch(maxLines, maxLineCharacters, maxFiles int, reader io.Reader) (*D
569609
break
570610
}
571611

572-
// Note: In case file name is surrounded by double quotes (it happens only in git-shell).
573-
// e.g. diff --git "a/xxx" "b/xxx"
574-
var a string
575-
var b string
576-
577-
rd := strings.NewReader(line[len(cmdDiffHead):])
578-
char, _ := rd.ReadByte()
579-
_ = rd.UnreadByte()
580-
if char == '"' {
581-
fmt.Fscanf(rd, "%q ", &a)
582-
} else {
583-
fmt.Fscanf(rd, "%s ", &a)
584-
}
585-
char, _ = rd.ReadByte()
586-
_ = rd.UnreadByte()
587-
if char == '"' {
588-
fmt.Fscanf(rd, "%q", &b)
589-
} else {
590-
fmt.Fscanf(rd, "%s", &b)
591-
}
592-
a = a[2:]
593-
b = b[2:]
594-
595612
curFile = &DiffFile{
596-
Name: b,
597-
OldName: a,
598-
Index: len(diff.Files) + 1,
599-
Type: DiffFileChange,
600-
Sections: make([]*DiffSection, 0, 10),
601-
IsRenamed: a != b,
613+
Index: len(diff.Files) + 1,
614+
Type: DiffFileChange,
615+
Sections: make([]*DiffSection, 0, 10),
602616
}
603617
diff.Files = append(diff.Files, curFile)
604618
curFileLinesCount = 0
@@ -607,6 +621,7 @@ func ParsePatch(maxLines, maxLineCharacters, maxFiles int, reader io.Reader) (*D
607621
curFileLFSPrefix = false
608622

609623
// Check file diff type and is submodule.
624+
loop:
610625
for {
611626
line, err := input.ReadString('\n')
612627
if err != nil {
@@ -617,23 +632,67 @@ func ParsePatch(maxLines, maxLineCharacters, maxFiles int, reader io.Reader) (*D
617632
}
618633
}
619634

620-
switch {
621-
case strings.HasPrefix(line, "new file"):
622-
curFile.Type = DiffFileAdd
623-
curFile.IsCreated = true
624-
case strings.HasPrefix(line, "deleted"):
625-
curFile.Type = DiffFileDel
626-
curFile.IsDeleted = true
627-
case strings.HasPrefix(line, "index"):
628-
curFile.Type = DiffFileChange
629-
case strings.HasPrefix(line, "similarity index 100%"):
630-
curFile.Type = DiffFileRename
631-
}
632-
if curFile.Type > 0 {
633-
if strings.HasSuffix(line, " 160000\n") {
634-
curFile.IsSubmodule = true
635+
if curFile.Type != DiffFileRename {
636+
switch {
637+
case strings.HasPrefix(line, "new file"):
638+
curFile.Type = DiffFileAdd
639+
curFile.IsCreated = true
640+
case strings.HasPrefix(line, "deleted"):
641+
curFile.Type = DiffFileDel
642+
curFile.IsDeleted = true
643+
case strings.HasPrefix(line, "index"):
644+
curFile.Type = DiffFileChange
645+
case strings.HasPrefix(line, "similarity index 100%"):
646+
curFile.Type = DiffFileRename
647+
}
648+
if curFile.Type > 0 && curFile.Type != DiffFileRename {
649+
if strings.HasSuffix(line, " 160000\n") {
650+
curFile.IsSubmodule = true
651+
}
652+
break
653+
}
654+
} else {
655+
switch {
656+
case strings.HasPrefix(line, "rename from "):
657+
if line[12] == '"' {
658+
fmt.Sscanf(line[12:], "%q", &curFile.OldName)
659+
} else {
660+
curFile.OldName = line[12:]
661+
curFile.OldName = curFile.OldName[:len(curFile.OldName)-1]
662+
}
663+
case strings.HasPrefix(line, "rename to "):
664+
if line[10] == '"' {
665+
fmt.Sscanf(line[10:], "%q", &curFile.Name)
666+
} else {
667+
curFile.Name = line[10:]
668+
curFile.Name = curFile.Name[:len(curFile.Name)-1]
669+
}
670+
curFile.IsRenamed = true
671+
break loop
672+
case strings.HasPrefix(line, "copy from "):
673+
if line[10] == '"' {
674+
fmt.Sscanf(line[10:], "%q", &curFile.OldName)
675+
} else {
676+
curFile.OldName = line[10:]
677+
curFile.OldName = curFile.OldName[:len(curFile.OldName)-1]
678+
}
679+
case strings.HasPrefix(line, "copy to "):
680+
if line[8] == '"' {
681+
fmt.Sscanf(line[8:], "%q", &curFile.Name)
682+
} else {
683+
curFile.Name = line[8:]
684+
curFile.Name = curFile.Name[:len(curFile.Name)-1]
685+
}
686+
curFile.IsRenamed = true
687+
curFile.Type = DiffFileCopy
688+
break loop
689+
default:
690+
if strings.HasSuffix(line, " 160000\n") {
691+
curFile.IsSubmodule = true
692+
} else {
693+
break loop
694+
}
635695
}
636-
break
637696
}
638697
}
639698
}

‎services/gitdiff/gitdiff_test.go

+141-4
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"
@@ -14,11 +15,9 @@ import (
1415
"code.gitea.io/gitea/models"
1516
"code.gitea.io/gitea/modules/git"
1617
"code.gitea.io/gitea/modules/setting"
17-
18-
"gopkg.in/ini.v1"
19-
2018
dmp "github.com/sergi/go-diff/diffmatchpatch"
2119
"github.com/stretchr/testify/assert"
20+
"gopkg.in/ini.v1"
2221
)
2322

2423
func assertEqual(t *testing.T, s1 string, s2 template.HTML) {
@@ -77,7 +76,145 @@ func TestDiffToHTML(t *testing.T) {
7776
}, DiffLineAdd))
7877
}
7978

80-
func TestParsePatch(t *testing.T) {
79+
func TestParsePatch_singlefile(t *testing.T) {
80+
type testcase struct {
81+
name string
82+
gitdiff string
83+
wantErr bool
84+
addition int
85+
deletion int
86+
oldFilename string
87+
filename string
88+
}
89+
90+
tests := []testcase{
91+
{
92+
name: "readme.md2readme.md",
93+
gitdiff: `diff --git "a/README.md" "b/README.md"
94+
--- a/README.md
95+
+++ b/README.md
96+
@@ -1,3 +1,6 @@
97+
# gitea-github-migrator
98+
+
99+
+ Build Status
100+
- Latest Release
101+
Docker Pulls
102+
+ cut off
103+
+ cut off
104+
`,
105+
addition: 4,
106+
deletion: 1,
107+
filename: "README.md",
108+
},
109+
{
110+
name: "A \\ B",
111+
gitdiff: `diff --git "a/A \\ B" "b/A \\ B"
112+
--- "a/A \\ B"
113+
+++ "b/A \\ B"
114+
@@ -1,3 +1,6 @@
115+
# gitea-github-migrator
116+
+
117+
+ Build Status
118+
- Latest Release
119+
Docker Pulls
120+
+ cut off
121+
+ cut off`,
122+
addition: 4,
123+
deletion: 1,
124+
filename: "A \\ B",
125+
},
126+
{
127+
name: "really weird filename",
128+
gitdiff: `diff --git a/a b/file b/a a/file b/a b/file b/a a/file
129+
index d2186f1..f5c8ed2 100644
130+
--- a/a b/file b/a a/file
131+
+++ b/a b/file b/a a/file
132+
@@ -1,3 +1,2 @@
133+
Create a weird file.
134+
135+
-and what does diff do here?
136+
\ No newline at end of file`,
137+
addition: 0,
138+
deletion: 1,
139+
filename: "a b/file b/a a/file",
140+
oldFilename: "a b/file b/a a/file",
141+
},
142+
{
143+
name: "delete file with blanks",
144+
gitdiff: `diff --git a/file with blanks b/file with blanks
145+
deleted file mode 100644
146+
index 898651a..0000000
147+
--- a/file with blanks
148+
+++ /dev/null
149+
@@ -1,5 +0,0 @@
150+
-a blank file
151+
-
152+
-has a couple o line
153+
-
154+
-the 5th line is the last
155+
`,
156+
addition: 0,
157+
deletion: 5,
158+
filename: "file with blanks",
159+
},
160+
{
161+
name: "rename a—as",
162+
gitdiff: `diff --git "a/\360\243\220\265b\342\200\240vs" "b/a\342\200\224as"
163+
similarity index 100%
164+
rename from "\360\243\220\265b\342\200\240vs"
165+
rename to "a\342\200\224as"
166+
`,
167+
addition: 0,
168+
deletion: 0,
169+
oldFilename: "𣐵b†vs",
170+
filename: "a—as",
171+
},
172+
{
173+
name: "rename with spaces",
174+
gitdiff: `diff --git a/a b/file b/a a/file b/a b/a a/file b/b file
175+
similarity index 100%
176+
rename from a b/file b/a a/file
177+
rename to a b/a a/file b/b file
178+
`,
179+
oldFilename: "a b/file b/a a/file",
180+
filename: "a b/a a/file b/b file",
181+
},
182+
}
183+
184+
for _, testcase := range tests {
185+
t.Run(testcase.name, func(t *testing.T) {
186+
got, err := ParsePatch(setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(testcase.gitdiff))
187+
if (err != nil) != testcase.wantErr {
188+
t.Errorf("ParsePatch() error = %v, wantErr %v", err, testcase.wantErr)
189+
return
190+
}
191+
gotMarshaled, _ := json.MarshalIndent(got, " ", " ")
192+
if got.NumFiles != 1 {
193+
t.Errorf("ParsePath() did not receive 1 file:\n%s", string(gotMarshaled))
194+
return
195+
}
196+
if got.TotalAddition != testcase.addition {
197+
t.Errorf("ParsePath() does not have correct totalAddition %d, wanted %d", got.TotalAddition, testcase.addition)
198+
}
199+
if got.TotalDeletion != testcase.deletion {
200+
t.Errorf("ParsePath() did not have correct totalDeletion %d, wanted %d", got.TotalDeletion, testcase.deletion)
201+
}
202+
file := got.Files[0]
203+
if file.Addition != testcase.addition {
204+
t.Errorf("ParsePath() does not have correct file addition %d, wanted %d", file.Addition, testcase.addition)
205+
}
206+
if file.Deletion != testcase.deletion {
207+
t.Errorf("ParsePath() did not have correct file deletion %d, wanted %d", file.Deletion, testcase.deletion)
208+
}
209+
if file.OldName != testcase.oldFilename {
210+
t.Errorf("ParsePath() did not have correct OldName %s, wanted %s", file.OldName, testcase.oldFilename)
211+
}
212+
if file.Name != testcase.filename {
213+
t.Errorf("ParsePath() did not have correct Name %s, wanted %s", file.Name, testcase.filename)
214+
}
215+
})
216+
}
217+
81218
var diff = `diff --git "a/README.md" "b/README.md"
82219
--- a/README.md
83220
+++ b/README.md

0 commit comments

Comments
 (0)