Skip to content

Commit 5a0f570

Browse files
committed
fix
1 parent 45ee571 commit 5a0f570

File tree

4 files changed

+77
-43
lines changed

4 files changed

+77
-43
lines changed

modules/git/catfile_batch_test.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,9 @@
44
package git
55

66
import (
7+
"errors"
78
"io"
9+
"os"
810
"path/filepath"
911
"sync"
1012
"testing"
@@ -81,7 +83,10 @@ func testCatFileBatch(t *testing.T) {
8183
_, _ = c.respReader.Read(buf)
8284
n, errRead := c.respReader.Read(buf)
8385
assert.Zero(t, n)
84-
assert.ErrorIs(t, errRead, io.EOF) // the pipe is closed due to command being killed
86+
// the pipe is closed due to command being killed
87+
if errOK := errors.Is(err, os.ErrClosed) || errors.Is(err, io.EOF); !errOK {
88+
assert.Fail(t, "unexpected error", "error: %v", errRead)
89+
}
8590
})
8691
c.debugGitCmd.DebugKill()
8792
wg.Wait()

services/gitdiff/gitdiff.go

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616
"path"
1717
"sort"
1818
"strings"
19+
"time"
1920

2021
"code.gitea.io/gitea/models/db"
2122
git_model "code.gitea.io/gitea/models/git"
@@ -123,8 +124,13 @@ type DiffHTMLOperation struct {
123124
// BlobExcerptChunkSize represent max lines of excerpt
124125
const BlobExcerptChunkSize = 20
125126

126-
// MaxDiffHighlightEntireFileSize is the maximum file size that will be highlighted with "entire file diff"
127-
const MaxDiffHighlightEntireFileSize = 1 * 1024 * 1024
127+
// Chroma seems extremely slow when highlighting large files, it might take dozens or hundreds of milliseconds.
128+
// When fully highlighting a diff with a lot of large files, it would take many seconds or even dozens of seconds.
129+
// So, don't highlight the entire file if it's too large, or highlighting takes too long.
130+
const (
131+
MaxFullFileHighlightSizeLimit = 256 * 1024
132+
MaxFullFileHighlightTimeLimit = 2 * time.Second
133+
)
128134

129135
// GetType returns the type of DiffLine.
130136
func (d *DiffLine) GetType() int {
@@ -564,7 +570,7 @@ func getCommitFileLineCountAndLimitedContent(commit *git.Commit, filePath string
564570
if err != nil {
565571
return 0, nil
566572
}
567-
w := &limitByteWriter{limit: MaxDiffHighlightEntireFileSize + 1}
573+
w := &limitByteWriter{limit: MaxFullFileHighlightSizeLimit + 1}
568574
lineCount, err = blob.GetBlobLineCount(w)
569575
if err != nil {
570576
return 0, nil
@@ -1317,6 +1323,8 @@ func GetDiffForRender(ctx context.Context, repoLink string, gitRepo *git.Reposit
13171323
return nil, err
13181324
}
13191325

1326+
startTime := time.Now()
1327+
13201328
checker, err := attribute.NewBatchChecker(gitRepo, opts.AfterCommitID, []string{attribute.LinguistVendored, attribute.LinguistGenerated, attribute.LinguistLanguage, attribute.GitlabLanguage, attribute.Diff})
13211329
if err != nil {
13221330
return nil, err
@@ -1356,7 +1364,8 @@ func GetDiffForRender(ctx context.Context, repoLink string, gitRepo *git.Reposit
13561364
diffFile.Sections = append(diffFile.Sections, tailSection)
13571365
}
13581366

1359-
shouldFullFileHighlight := !setting.Git.DisableDiffHighlight && attrDiff.Value() == ""
1367+
shouldFullFileHighlight := attrDiff.Value() == "" // only do highlight if no custom diff command
1368+
shouldFullFileHighlight = shouldFullFileHighlight && time.Since(startTime) < MaxFullFileHighlightTimeLimit
13601369
if shouldFullFileHighlight {
13611370
if limitedContent.LeftContent != nil {
13621371
diffFile.highlightedLeftLines.value = highlightCodeLinesForDiffFile(diffFile, true /* left */, limitedContent.LeftContent.buf.Bytes())
@@ -1380,7 +1389,7 @@ func highlightCodeLinesForDiffFile(diffFile *DiffFile, isLeft bool, rawContent [
13801389
}
13811390

13821391
func highlightCodeLines(name, lang string, sections []*DiffSection, isLeft bool, rawContent []byte) map[int]template.HTML {
1383-
if setting.Git.DisableDiffHighlight || len(rawContent) > MaxDiffHighlightEntireFileSize {
1392+
if setting.Git.DisableDiffHighlight || len(rawContent) > MaxFullFileHighlightSizeLimit {
13841393
return nil
13851394
}
13861395

services/gitdiff/highlightdiff.go

Lines changed: 54 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -298,15 +298,21 @@ func (hcd *highlightCodeDiff) convertToPlaceholders(htmlContent template.HTML) s
298298
return res.String()
299299
}
300300

301-
func (hcd *highlightCodeDiff) extractNextPlaceholder(buf []byte, lastIdx int) (idx int, placeholder rune, runeLen int, token string) {
302-
for idx = lastIdx; idx < len(buf); {
303-
placeholder, runeLen = utf8.DecodeRune(buf[idx:])
304-
if token = hcd.placeholderTokenMap[placeholder]; token != "" {
305-
break
301+
// recoverOneRune tries to recover one rune
302+
// * if the rune is a placeholder, it will be recovered to the corresponding content
303+
// * otherwise it will be returned as is
304+
func (hcd *highlightCodeDiff) recoverOneRune(buf []byte) (r rune, runeLen int, isSingleTag bool, recovered string) {
305+
r, runeLen = utf8.DecodeRune(buf)
306+
token := hcd.placeholderTokenMap[r]
307+
if token == "" {
308+
return r, runeLen, false, "" // rune itself, not a placeholder
309+
} else if token[0] == '<' {
310+
if token[1] == '<' {
311+
return 0, runeLen, false, token[1 : len(token)-1] // full tag `<<span>content</span>>`, recover to `<span>content</span>`
306312
}
307-
idx += runeLen
313+
return r, runeLen, true, token // single tag
308314
}
309-
return idx, placeholder, runeLen, token
315+
return 0, runeLen, false, token // HTML entity
310316
}
311317

312318
func (hcd *highlightCodeDiff) recoverOneDiff(str string) template.HTML {
@@ -315,49 +321,65 @@ func (hcd *highlightCodeDiff) recoverOneDiff(str string) template.HTML {
315321
var diffCodeOpenTag string
316322
diffCodeCloseTag := hcd.placeholderTokenMap[hcd.diffCodeClose]
317323
strBytes := util.UnsafeStringToBytes(str)
324+
325+
// this loop is slightly longer than expected, for performance consideration
318326
for idx := 0; idx < len(strBytes); {
319-
newIdx, placeholder, lastRuneLen, token := hcd.extractNextPlaceholder(strBytes, idx)
320-
if newIdx != idx {
327+
// take a look at the next rune
328+
r, runeLen, isSingleTag, recovered := hcd.recoverOneRune(strBytes[idx:])
329+
idx += runeLen
330+
331+
// loop section 1: if it isn't a single tag, then try to find the following runes until the next single tag, and recover them together
332+
if !isSingleTag {
321333
if diffCodeOpenTag != "" {
334+
// start the "added/removed diff tag" if the current token is in the diff part
322335
sb.WriteString(diffCodeOpenTag)
323-
sb.Write(strBytes[idx:newIdx])
324-
sb.WriteString(diffCodeCloseTag)
336+
}
337+
if recovered != "" {
338+
sb.WriteString(recovered)
325339
} else {
326-
sb.Write(strBytes[idx:newIdx])
340+
sb.WriteRune(r)
341+
}
342+
// inner loop to recover following runes until the next single tag
343+
for idx < len(strBytes) {
344+
r, runeLen, isSingleTag, recovered = hcd.recoverOneRune(strBytes[idx:])
345+
idx += runeLen
346+
if isSingleTag {
347+
break
348+
}
349+
if recovered != "" {
350+
sb.WriteString(recovered)
351+
} else {
352+
sb.WriteRune(r)
353+
}
354+
}
355+
if diffCodeOpenTag != "" {
356+
// end the "added/removed diff tag" if the current token is in the diff part
357+
sb.WriteString(diffCodeCloseTag)
327358
}
328-
} // else: no text content before, the current token is a placeholder
329-
if token == "" {
330-
break // reaches the string end, no more placeholder
331359
}
332-
idx = newIdx + lastRuneLen
333360

334-
// for HTML entity
335-
if token[0] == '&' {
336-
sb.WriteString(token)
337-
continue
361+
if !isSingleTag {
362+
break // the inner loop has already consumed all remaining runes, no more single tag found
338363
}
339364

340-
// for various HTML tags
341-
var recovered string
342-
if token[1] == '<' { // full tag `<<span>content</span>>`, recover to `<span>content</span>`
343-
recovered = token[1 : len(token)-1]
344-
if diffCodeOpenTag != "" {
345-
recovered = diffCodeOpenTag + recovered + diffCodeCloseTag
346-
} // else: just use the recovered content
347-
} else if token[1] != '/' { // opening tag
365+
// loop section 2: for opening/closing HTML tags
366+
placeholder := r
367+
if recovered[1] != '/' { // opening tag
348368
if placeholder == hcd.diffCodeAddedOpen || placeholder == hcd.diffCodeRemovedOpen {
349-
diffCodeOpenTag = token
369+
diffCodeOpenTag = recovered
370+
recovered = ""
350371
} else {
351-
recovered = token
352372
tagStack = append(tagStack, recovered)
353373
}
354374
} else { // closing tag
355375
if placeholder == hcd.diffCodeClose {
356376
diffCodeOpenTag = "" // the highlighted diff is closed, no more diff
377+
recovered = ""
357378
} else if len(tagStack) != 0 {
358-
recovered = token
359379
tagStack = tagStack[:len(tagStack)-1]
360-
} // else: if no opening tag in stack yet, skip the closing tag
380+
} else {
381+
recovered = ""
382+
}
361383
}
362384
sb.WriteString(recovered)
363385
}

services/gitdiff/highlightdiff_test.go

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -77,15 +77,13 @@ func TestDiffWithHighlight(t *testing.T) {
7777

7878
t.Run("ComplexDiff1", func(t *testing.T) {
7979
oldCode, _ := highlight.RenderCodeFast("a.go", "Go", `xxx || yyy`)
80-
newCode, _ := highlight.RenderCodeFast("a.go", "Go", `bot.xxx || bot.yyy`)
80+
newCode, _ := highlight.RenderCodeFast("a.go", "Go", `bot&xxx || bot&yyy`)
8181
hcd := newHighlightCodeDiff()
8282
out := hcd.diffLineWithHighlight(DiffLineAdd, oldCode, newCode)
8383
assert.Equal(t, strings.ReplaceAll(`
84-
<span class="added-code"><span class="nx">bot</span></span>
85-
<span class="added-code"><span class="p">.</span></span>
84+
<span class="added-code"><span class="nx">bot</span></span><span class="o"><span class="added-code">&amp;</span></span>
8685
<span class="nx">xxx</span><span class="w"> </span><span class="o">||</span><span class="w"> </span>
87-
<span class="added-code"><span class="nx">bot</span></span>
88-
<span class="added-code"><span class="p">.</span></span>
86+
<span class="added-code"><span class="nx">bot</span></span><span class="o"><span class="added-code">&amp;</span></span>
8987
<span class="nx">yyy</span>`, "\n", ""), string(out))
9088
})
9189

0 commit comments

Comments
 (0)