* 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
Showing the changes to services/gitdiff/gitdiff.go (1 of 2 files). Show all
Unified · Split
| @@ -181,64 +181,60 @@ var ( · 180 unmodified lines | ||
| 181 | 181 | removedCodePrefix = []byte(`<span class="removed-code">`) |
| 182 | 182 | codeTagSuffix = []byte(`</span>`) |
| 183 | 183 | ) |
| 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 | +} | |
| 185 | 196 | |
| 186 | 197 | func diffToHTML(fileName string, diffs []diffmatchpatch.Diff, lineType DiffLineType) template.HTML { |
| 187 | 198 | 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 = "" | |
| 199 | 206 | } |
| 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) | |
| 201 | 212 | 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) | |
| 212 | 215 | } |
| 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>") { | |
| 215 | 218 | buf.WriteString("</span>") |
| 216 | − diffs[i].Text = strings.TrimPrefix(diffs[i].Text, "</span>") | |
| 219 | + diff.Text = strings.TrimPrefix(diff.Text, "</span>") | |
| 217 | 220 | } |
| 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 | |
| 222 | 226 | } |
| 227 | + } | |
| 228 | + switch { | |
| 229 | + case diff.Type == diffmatchpatch.DiffEqual: | |
| 230 | + buf.WriteString(diff.Text) | |
| 231 | + case diff.Type == diffmatchpatch.DiffInsert && lineType == DiffLineAdd: | |
| 223 | 232 | buf.Write(addedCodePrefix) |
| 224 | − buf.WriteString(diffs[i].Text) | |
| 233 | + buf.WriteString(diff.Text) | |
| 225 | 234 | 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: | |
| 240 | 236 | buf.Write(removedCodePrefix) |
| 241 | − buf.WriteString(diffs[i].Text) | |
| 237 | + buf.WriteString(diff.Text) | |
| 242 | 238 | buf.Write(codeTagSuffix) |
| 243 | 239 | } |
| 244 | 240 | } |