Skip to content

Commit f6ee7ce

Browse files
Add better error checking for inline html diff code (#13239)
* Add better error checking for inline html diff code A better fix for #13191 which cleans up this code a bit and adds basic checking which should avoid writing broken HTML in future situations. * Update gitdiff_test.go * better regex Co-authored-by: techknowlogick <[email protected]>
1 parent 83106c1 commit f6ee7ce

File tree

2 files changed

+44
-48
lines changed

2 files changed

+44
-48
lines changed

services/gitdiff/gitdiff.go

+42-46
Original file line numberDiff line numberDiff line change
@@ -181,64 +181,60 @@ var (
181181
removedCodePrefix = []byte(`<span class="removed-code">`)
182182
codeTagSuffix = []byte(`</span>`)
183183
)
184-
var addSpanRegex = regexp.MustCompile(`<span\s*[a-z="]*$`)
184+
var trailingSpanRegex = regexp.MustCompile(`<span\s*[[:alpha:]="]*?[>]?$`)
185+
186+
// shouldWriteInline represents combinations where we manually write inline changes
187+
func shouldWriteInline(diff diffmatchpatch.Diff, lineType DiffLineType) bool {
188+
if true &&
189+
diff.Type == diffmatchpatch.DiffEqual ||
190+
diff.Type == diffmatchpatch.DiffInsert && lineType == DiffLineAdd ||
191+
diff.Type == diffmatchpatch.DiffDelete && lineType == DiffLineDel {
192+
return true
193+
}
194+
return false
195+
}
185196

186197
func diffToHTML(fileName string, diffs []diffmatchpatch.Diff, lineType DiffLineType) template.HTML {
187198
buf := bytes.NewBuffer(nil)
188-
var addSpan string
189-
for i := range diffs {
190-
switch {
191-
case diffs[i].Type == diffmatchpatch.DiffEqual:
192-
// Looking for the case where our 3rd party diff library previously detected a string difference
193-
// in the middle of a span class because we highlight them first. This happens when added/deleted code
194-
// also changes the chroma class name, either partially or fully. If found, just move the openining span code forward into the next section
195-
// see TestDiffToHTML for examples
196-
if len(addSpan) > 0 {
197-
diffs[i].Text = addSpan + diffs[i].Text
198-
addSpan = ""
199+
match := ""
200+
201+
for _, diff := range diffs {
202+
if shouldWriteInline(diff, lineType) {
203+
if len(match) > 0 {
204+
diff.Text = match + diff.Text
205+
match = ""
199206
}
200-
m := addSpanRegex.FindStringSubmatchIndex(diffs[i].Text)
207+
// Chroma HTML syntax highlighting is done before diffing individual lines in order to maintain consistency.
208+
// Since inline changes might split in the middle of a chroma span tag, make we manually put it back together
209+
// before writing so we don't try insert added/removed code spans in the middle of an existing chroma span
210+
// and create broken HTML.
211+
m := trailingSpanRegex.FindStringSubmatchIndex(diff.Text)
201212
if m != nil {
202-
addSpan = diffs[i].Text[m[0]:m[1]]
203-
buf.WriteString(strings.TrimSuffix(diffs[i].Text, addSpan))
204-
} else {
205-
addSpan = ""
206-
buf.WriteString(getLineContent(diffs[i].Text))
207-
}
208-
case diffs[i].Type == diffmatchpatch.DiffInsert && lineType == DiffLineAdd:
209-
if len(addSpan) > 0 {
210-
diffs[i].Text = addSpan + diffs[i].Text
211-
addSpan = ""
213+
match = diff.Text[m[0]:m[1]]
214+
diff.Text = strings.TrimSuffix(diff.Text, match)
212215
}
213-
// Print existing closing span first before opening added-code span so it doesn't unintentionally close it
214-
if strings.HasPrefix(diffs[i].Text, "</span>") {
216+
// Print an existing closing span first before opening added/remove-code span so it doesn't unintentionally close it
217+
if strings.HasPrefix(diff.Text, "</span>") {
215218
buf.WriteString("</span>")
216-
diffs[i].Text = strings.TrimPrefix(diffs[i].Text, "</span>")
219+
diff.Text = strings.TrimPrefix(diff.Text, "</span>")
217220
}
218-
m := addSpanRegex.FindStringSubmatchIndex(diffs[i].Text)
219-
if m != nil {
220-
addSpan = diffs[i].Text[m[0]:m[1]]
221-
diffs[i].Text = strings.TrimSuffix(diffs[i].Text, addSpan)
221+
// If we weren't able to fix it then this should avoid broken HTML by not inserting more spans below
222+
// The previous/next diff section will contain the rest of the tag that is missing here
223+
if strings.Count(diff.Text, "<") != strings.Count(diff.Text, ">") {
224+
buf.WriteString(diff.Text)
225+
continue
222226
}
227+
}
228+
switch {
229+
case diff.Type == diffmatchpatch.DiffEqual:
230+
buf.WriteString(diff.Text)
231+
case diff.Type == diffmatchpatch.DiffInsert && lineType == DiffLineAdd:
223232
buf.Write(addedCodePrefix)
224-
buf.WriteString(diffs[i].Text)
233+
buf.WriteString(diff.Text)
225234
buf.Write(codeTagSuffix)
226-
case diffs[i].Type == diffmatchpatch.DiffDelete && lineType == DiffLineDel:
227-
if len(addSpan) > 0 {
228-
diffs[i].Text = addSpan + diffs[i].Text
229-
addSpan = ""
230-
}
231-
if strings.HasPrefix(diffs[i].Text, "</span>") {
232-
buf.WriteString("</span>")
233-
diffs[i].Text = strings.TrimPrefix(diffs[i].Text, "</span>")
234-
}
235-
m := addSpanRegex.FindStringSubmatchIndex(diffs[i].Text)
236-
if m != nil {
237-
addSpan = diffs[i].Text[m[0]:m[1]]
238-
diffs[i].Text = strings.TrimSuffix(diffs[i].Text, addSpan)
239-
}
235+
case diff.Type == diffmatchpatch.DiffDelete && lineType == DiffLineDel:
240236
buf.Write(removedCodePrefix)
241-
buf.WriteString(diffs[i].Text)
237+
buf.WriteString(diff.Text)
242238
buf.Write(codeTagSuffix)
243239
}
244240
}

services/gitdiff/gitdiff_test.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ func TestDiffToHTML(t *testing.T) {
5050
{Type: dmp.DiffEqual, Text: "</span> <span class=\"p\">{</span>"},
5151
}, DiffLineAdd))
5252

53-
assertEqual(t, "<span class=\"nx\">tagURL</span> <span class=\"o\">:=</span> <span class=\"removed-code\"><span class=\"nx\">fmt</span><span class=\"p\">.</span><span class=\"nf\">Sprintf</span><span class=\"p\">(</span><span class=\"s\">&#34;## [%s](%s/%s/%s/%s?q=&amp;type=all&amp;state=closed&amp;milestone=%d) - %s&#34;</span><span class=\"p\">,</span> <span class=\"nx\">ge</span><span class=\"p\">.</span><span class=\"nx\">Milestone\"</span></span><span class=\"p\">,</span> <span class=\"nx\">ge</span><span class=\"p\">.</span><span class=\"nx\">BaseURL</span><span class=\"p\">,</span> <span class=\"nx\">ge</span><span class=\"p\">.</span><span class=\"nx\">Owner</span><span class=\"p\">,</span> <span class=\"nx\">ge</span><span class=\"p\">.</span><span class=\"nx\">Repo</span><span class=\"p\">,</span> <span class=\"nx\"><span class=\"removed-code\">from</span><span class=\"p\">,</span> <span class=\"nx\">milestoneID</span><span class=\"p\">,</span> <span class=\"nx\">time</span><span class=\"p\">.</span><span class=\"nf\">Now</span><span class=\"p\">(</span><span class=\"p\">)</span><span class=\"p\">.</span><span class=\"nf\">Format</span><span class=\"p\">(</span><span class=\"s\">&#34;2006-01-02&#34;</span><span class=\"p\">)</span></span><span class=\"p\">)</span>", diffToHTML("", []dmp.Diff{
53+
assertEqual(t, "<span class=\"nx\">tagURL</span> <span class=\"o\">:=</span> <span class=\"removed-code\"><span class=\"nx\">fmt</span><span class=\"p\">.</span><span class=\"nf\">Sprintf</span><span class=\"p\">(</span><span class=\"s\">&#34;## [%s](%s/%s/%s/%s?q=&amp;type=all&amp;state=closed&amp;milestone=%d) - %s&#34;</span><span class=\"p\">,</span> <span class=\"nx\">ge</span><span class=\"p\">.</span><span class=\"nx\">Milestone\"</span></span><span class=\"p\">,</span> <span class=\"nx\">ge</span><span class=\"p\">.</span><span class=\"nx\">BaseURL</span><span class=\"p\">,</span> <span class=\"nx\">ge</span><span class=\"p\">.</span><span class=\"nx\">Owner</span><span class=\"p\">,</span> <span class=\"nx\">ge</span><span class=\"p\">.</span><span class=\"nx\">Repo</span><span class=\"p\">,</span> <span class=\"removed-code\"><span class=\"nx\">from</span><span class=\"p\">,</span> <span class=\"nx\">milestoneID</span><span class=\"p\">,</span> <span class=\"nx\">time</span><span class=\"p\">.</span><span class=\"nf\">Now</span><span class=\"p\">(</span><span class=\"p\">)</span><span class=\"p\">.</span><span class=\"nf\">Format</span><span class=\"p\">(</span><span class=\"s\">&#34;2006-01-02&#34;</span><span class=\"p\">)</span></span><span class=\"p\">)</span>", diffToHTML("", []dmp.Diff{
5454
{Type: dmp.DiffEqual, Text: "<span class=\"nx\">tagURL</span> <span class=\"o\">:=</span> <span class=\"n"},
5555
{Type: dmp.DiffDelete, Text: "x\">fmt</span><span class=\"p\">.</span><span class=\"nf\">Sprintf</span><span class=\"p\">(</span><span class=\"s\">&#34;## [%s](%s/%s/%s/%s?q=&amp;type=all&amp;state=closed&amp;milestone=%d) - %s&#34;</span><span class=\"p\">,</span> <span class=\"nx\">ge</span><span class=\"p\">.</span><span class=\"nx\">Milestone\""},
5656
{Type: dmp.DiffInsert, Text: "f\">getGiteaTagURL</span><span class=\"p\">(</span><span class=\"nx\">client"},
@@ -60,7 +60,7 @@ func TestDiffToHTML(t *testing.T) {
6060
{Type: dmp.DiffEqual, Text: "</span><span class=\"p\">)</span>"},
6161
}, DiffLineDel))
6262

63-
assertEqual(t, "<span class=\"nx\">r</span><span class=\"p\">.</span><span class=\"nf\">WrapperRenderer</span><span class=\"p\">(</span><span class=\"nx\">w</span><span class=\"p\">,</span> <span class=\"nx\"><span class=\"removed-code\">language</span></span><span class=\"removed-code\"><span class=\"p\">,</span> <span class=\"kc\">true</span><span class=\"p\">,</span> <span class=\"nx\">attrs</span></span><span class=\"p\">,</span> <span class=\"kc\">false</span><span class=\"p\">)</span>", diffToHTML("", []dmp.Diff{
63+
assertEqual(t, "<span class=\"nx\">r</span><span class=\"p\">.</span><span class=\"nf\">WrapperRenderer</span><span class=\"p\">(</span><span class=\"nx\">w</span><span class=\"p\">,</span> <span class=\"removed-code\"><span class=\"nx\">language</span></span><span class=\"removed-code\"><span class=\"p\">,</span> <span class=\"kc\">true</span><span class=\"p\">,</span> <span class=\"nx\">attrs</span></span><span class=\"p\">,</span> <span class=\"kc\">false</span><span class=\"p\">)</span>", diffToHTML("", []dmp.Diff{
6464
{Type: dmp.DiffEqual, Text: "<span class=\"nx\">r</span><span class=\"p\">.</span><span class=\"nf\">WrapperRenderer</span><span class=\"p\">(</span><span class=\"nx\">w</span><span class=\"p\">,</span> <span class=\"nx\">"},
6565
{Type: dmp.DiffDelete, Text: "language</span><span "},
6666
{Type: dmp.DiffEqual, Text: "c"},

0 commit comments

Comments
 (0)