diff --git a/modules/highlight/highlight.go b/modules/highlight/highlight.go index 68f523c6ca..fc8699829c 100644 --- a/modules/highlight/highlight.go +++ b/modules/highlight/highlight.go @@ -98,6 +98,10 @@ func getChromaLexerByLanguage(fileName, lang string) chroma.Lexer { lang = "C++" } } + if lang == "" && util.AsciiEqualFold(ext, ".sql") { + // there is a bug when using MySQL lexer: "--\nSELECT", the second line will be rendered as comment incorrectly + lang = "SQL" + } // lexers.Get is slow if the language name can't be matched directly: it does extra "Match" call to iterate all lexers return lexers.Get(lang) } diff --git a/modules/highlight/highlight_test.go b/modules/highlight/highlight_test.go index f4bdedb2a0..69aff07b04 100644 --- a/modules/highlight/highlight_test.go +++ b/modules/highlight/highlight_test.go @@ -108,6 +108,12 @@ c=2 ), lexerName: "Python", }, + { + name: "test.sql", + code: "--\nSELECT", + want: []template.HTML{"--\n", `SELECT`}, + lexerName: "SQL", + }, } for _, tt := range tests { diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index 65c203d510..1e2486f5f1 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -4,7 +4,6 @@ package repo import ( - "bufio" gocontext "context" "encoding/csv" "errors" @@ -744,13 +743,16 @@ func attachHiddenCommentIDs(section *gitdiff.DiffSection, lineComments map[int64 // ExcerptBlob render blob excerpt contents func ExcerptBlob(ctx *context.Context) { commitID := ctx.PathParam("sha") - lastLeft := ctx.FormInt("last_left") - lastRight := ctx.FormInt("last_right") - idxLeft := ctx.FormInt("left") - idxRight := ctx.FormInt("right") - leftHunkSize := ctx.FormInt("left_hunk_size") - rightHunkSize := ctx.FormInt("right_hunk_size") - direction := ctx.FormString("direction") + opts := gitdiff.BlobExcerptOptions{ + LastLeft: ctx.FormInt("last_left"), + LastRight: ctx.FormInt("last_right"), + LeftIndex: ctx.FormInt("left"), + RightIndex: ctx.FormInt("right"), + LeftHunkSize: ctx.FormInt("left_hunk_size"), + RightHunkSize: ctx.FormInt("right_hunk_size"), + Direction: ctx.FormString("direction"), + Language: ctx.FormString("filelang"), + } filePath := ctx.FormString("path") gitRepo := ctx.Repo.GitRepo @@ -770,61 +772,27 @@ func ExcerptBlob(ctx *context.Context) { diffBlobExcerptData.BaseLink = ctx.Repo.RepoLink + "/wiki/blob_excerpt" } - chunkSize := gitdiff.BlobExcerptChunkSize commit, err := gitRepo.GetCommit(commitID) if err != nil { - ctx.HTTPError(http.StatusInternalServerError, "GetCommit") + ctx.ServerError("GetCommit", err) return } - section := &gitdiff.DiffSection{ - FileName: filePath, - } - if direction == "up" && (idxLeft-lastLeft) > chunkSize { - idxLeft -= chunkSize - idxRight -= chunkSize - leftHunkSize += chunkSize - rightHunkSize += chunkSize - section.Lines, err = getExcerptLines(commit, filePath, idxLeft-1, idxRight-1, chunkSize) - } else if direction == "down" && (idxLeft-lastLeft) > chunkSize { - section.Lines, err = getExcerptLines(commit, filePath, lastLeft, lastRight, chunkSize) - lastLeft += chunkSize - lastRight += chunkSize - } else { - offset := -1 - if direction == "down" { - offset = 0 - } - section.Lines, err = getExcerptLines(commit, filePath, lastLeft, lastRight, idxRight-lastRight+offset) - leftHunkSize = 0 - rightHunkSize = 0 - idxLeft = lastLeft - idxRight = lastRight - } + blob, err := commit.Tree.GetBlobByPath(filePath) if err != nil { - ctx.HTTPError(http.StatusInternalServerError, "getExcerptLines") + ctx.ServerError("GetBlobByPath", err) return } - - newLineSection := &gitdiff.DiffLine{ - Type: gitdiff.DiffLineSection, - SectionInfo: &gitdiff.DiffLineSectionInfo{ - Path: filePath, - LastLeftIdx: lastLeft, - LastRightIdx: lastRight, - LeftIdx: idxLeft, - RightIdx: idxRight, - LeftHunkSize: leftHunkSize, - RightHunkSize: rightHunkSize, - }, + reader, err := blob.DataAsync() + if err != nil { + ctx.ServerError("DataAsync", err) + return } - if newLineSection.GetExpandDirection() != "" { - newLineSection.Content = fmt.Sprintf("@@ -%d,%d +%d,%d @@\n", idxLeft, leftHunkSize, idxRight, rightHunkSize) - switch direction { - case "up": - section.Lines = append([]*gitdiff.DiffLine{newLineSection}, section.Lines...) - case "down": - section.Lines = append(section.Lines, newLineSection) - } + defer reader.Close() + + section, err := gitdiff.BuildBlobExcerptDiffSection(filePath, reader, opts) + if err != nil { + ctx.ServerError("BuildBlobExcerptDiffSection", err) + return } diffBlobExcerptData.PullIssueIndex = ctx.FormInt64("pull_issue_index") @@ -865,37 +833,3 @@ func ExcerptBlob(ctx *context.Context) { ctx.HTML(http.StatusOK, tplBlobExcerpt) } - -func getExcerptLines(commit *git.Commit, filePath string, idxLeft, idxRight, chunkSize int) ([]*gitdiff.DiffLine, error) { - blob, err := commit.Tree.GetBlobByPath(filePath) - if err != nil { - return nil, err - } - reader, err := blob.DataAsync() - if err != nil { - return nil, err - } - defer reader.Close() - scanner := bufio.NewScanner(reader) - var diffLines []*gitdiff.DiffLine - for line := 0; line < idxRight+chunkSize; line++ { - if ok := scanner.Scan(); !ok { - break - } - if line < idxRight { - continue - } - lineText := scanner.Text() - diffLine := &gitdiff.DiffLine{ - LeftIdx: idxLeft + (line - idxRight) + 1, - RightIdx: line + 1, - Type: gitdiff.DiffLinePlain, - Content: " " + lineText, - } - diffLines = append(diffLines, diffLine) - } - if err = scanner.Err(); err != nil { - return nil, fmt.Errorf("getExcerptLines scan: %w", err) - } - return diffLines, nil -} diff --git a/routers/web/repo/editor_preview.go b/routers/web/repo/editor_preview.go index 14be5b72b6..ec1f41a013 100644 --- a/routers/web/repo/editor_preview.go +++ b/routers/web/repo/editor_preview.go @@ -6,12 +6,13 @@ package repo import ( "net/http" + "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/services/context" files_service "code.gitea.io/gitea/services/repository/files" ) func DiffPreviewPost(ctx *context.Context) { - content := ctx.FormString("content") + newContent := ctx.FormString("content") treePath := files_service.CleanGitTreePath(ctx.Repo.TreePath) if treePath == "" { ctx.HTTPError(http.StatusBadRequest, "file name to diff is invalid") @@ -27,7 +28,12 @@ func DiffPreviewPost(ctx *context.Context) { return } - diff, err := files_service.GetDiffPreview(ctx, ctx.Repo.Repository, ctx.Repo.BranchName, treePath, content) + oldContent, err := entry.Blob().GetBlobContent(setting.UI.MaxDisplayFileSize) + if err != nil { + ctx.ServerError("GetBlobContent", err) + return + } + diff, err := files_service.GetDiffPreview(ctx, ctx.Repo.Repository, ctx.Repo.BranchName, treePath, oldContent, newContent) if err != nil { ctx.ServerError("GetDiffPreview", err) return diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index 3728f50d21..a62177c45c 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -81,6 +81,8 @@ type DiffLine struct { // DiffLineSectionInfo represents diff line section meta data type DiffLineSectionInfo struct { + language *diffVarMutable[string] + Path string // These line "idx" are 1-based line numbers @@ -165,16 +167,19 @@ func (d *DiffLine) GetLineTypeMarker() string { } func (d *DiffLine) getBlobExcerptQuery() string { - query := fmt.Sprintf( + language := "" + if d.SectionInfo.language != nil { // for normal cases, it can't be nil, this check is only for some tests + language = d.SectionInfo.language.value + } + return fmt.Sprintf( "last_left=%d&last_right=%d&"+ "left=%d&right=%d&"+ "left_hunk_size=%d&right_hunk_size=%d&"+ - "path=%s", + "path=%s&filelang=%s", d.SectionInfo.LastLeftIdx, d.SectionInfo.LastRightIdx, d.SectionInfo.LeftIdx, d.SectionInfo.RightIdx, d.SectionInfo.LeftHunkSize, d.SectionInfo.RightHunkSize, - url.QueryEscape(d.SectionInfo.Path)) - return query + url.QueryEscape(d.SectionInfo.Path), url.QueryEscape(language)) } func (d *DiffLine) GetExpandDirection() string { @@ -266,11 +271,12 @@ func FillHiddenCommentIDsForDiffLine(line *DiffLine, lineComments map[int64][]*i line.SectionInfo.HiddenCommentIDs = hiddenCommentIDs } -func getDiffLineSectionInfo(treePath, line string, lastLeftIdx, lastRightIdx int) *DiffLineSectionInfo { +func newDiffLineSectionInfo(curFile *DiffFile, line string, lastLeftIdx, lastRightIdx int) *DiffLineSectionInfo { leftLine, leftHunk, rightLine, rightHunk := git.ParseDiffHunkString(line) return &DiffLineSectionInfo{ - Path: treePath, + Path: curFile.Name, + language: &curFile.language, LastLeftIdx: lastLeftIdx, LastRightIdx: lastRightIdx, LeftIdx: leftLine, @@ -290,7 +296,10 @@ func getLineContent(content string, locale translation.Locale) DiffInline { // DiffSection represents a section of a DiffFile. type DiffSection struct { - file *DiffFile + language *diffVarMutable[string] + highlightedLeftLines *diffVarMutable[map[int]template.HTML] + highlightedRightLines *diffVarMutable[map[int]template.HTML] + FileName string Lines []*DiffLine } @@ -339,9 +348,9 @@ func (diffSection *DiffSection) getDiffLineForRender(diffLineType DiffLineType, var fileLanguage string var highlightedLeftLines, highlightedRightLines map[int]template.HTML // when a "diff section" is manually prepared by ExcerptBlob, it doesn't have "file" information - if diffSection.file != nil { - fileLanguage = diffSection.file.Language - highlightedLeftLines, highlightedRightLines = diffSection.file.highlightedLeftLines, diffSection.file.highlightedRightLines + if diffSection.language != nil { + fileLanguage = diffSection.language.value + highlightedLeftLines, highlightedRightLines = diffSection.highlightedLeftLines.value, diffSection.highlightedRightLines.value } var lineHTML template.HTML @@ -392,6 +401,11 @@ func (diffSection *DiffSection) GetComputedInlineDiffFor(diffLine *DiffLine, loc } } +// diffVarMutable is a wrapper to make a variable mutable to be shared across structs +type diffVarMutable[T any] struct { + value T +} + // DiffFile represents a file diff. type DiffFile struct { // only used internally to parse Ambiguous filenames @@ -418,7 +432,6 @@ type DiffFile struct { IsIncompleteLineTooLong bool // will be filled by the extra loop in GitDiffForRender - Language string IsGenerated bool IsVendored bool SubmoduleDiffInfo *SubmoduleDiffInfo // IsSubmodule==true, then there must be a SubmoduleDiffInfo @@ -430,9 +443,10 @@ type DiffFile struct { IsViewed bool // User specific HasChangedSinceLastReview bool // User specific - // for render purpose only, will be filled by the extra loop in GitDiffForRender - highlightedLeftLines map[int]template.HTML - highlightedRightLines map[int]template.HTML + // for render purpose only, will be filled by the extra loop in GitDiffForRender, the maps of lines are 0-based + language diffVarMutable[string] + highlightedLeftLines diffVarMutable[map[int]template.HTML] + highlightedRightLines diffVarMutable[map[int]template.HTML] } // GetType returns type of diff file. @@ -469,6 +483,7 @@ func (diffFile *DiffFile) GetTailSectionAndLimitedContent(leftCommit, rightCommi Type: DiffLineSection, Content: " ", SectionInfo: &DiffLineSectionInfo{ + language: &diffFile.language, Path: diffFile.Name, LastLeftIdx: lastLine.LeftIdx, LastRightIdx: lastLine.RightIdx, @@ -907,6 +922,14 @@ func skipToNextDiffHead(input *bufio.Reader) (line string, err error) { return line, err } +func newDiffSectionForDiffFile(curFile *DiffFile) *DiffSection { + return &DiffSection{ + language: &curFile.language, + highlightedLeftLines: &curFile.highlightedLeftLines, + highlightedRightLines: &curFile.highlightedRightLines, + } +} + func parseHunks(ctx context.Context, curFile *DiffFile, maxLines, maxLineCharacters int, input *bufio.Reader) (lineBytes []byte, isFragment bool, err error) { sb := strings.Builder{} @@ -964,12 +987,12 @@ func parseHunks(ctx context.Context, curFile *DiffFile, maxLines, maxLineCharact line := sb.String() // Create a new section to represent this hunk - curSection = &DiffSection{file: curFile} + curSection = newDiffSectionForDiffFile(curFile) lastLeftIdx = -1 curFile.Sections = append(curFile.Sections, curSection) // FIXME: the "-1" can't be right, these "line idx" are all 1-based, maybe there are other bugs that covers this bug. - lineSectionInfo := getDiffLineSectionInfo(curFile.Name, line, leftLine-1, rightLine-1) + lineSectionInfo := newDiffLineSectionInfo(curFile, line, leftLine-1, rightLine-1) diffLine := &DiffLine{ Type: DiffLineSection, Content: line, @@ -1004,7 +1027,7 @@ func parseHunks(ctx context.Context, curFile *DiffFile, maxLines, maxLineCharact rightLine++ if curSection == nil { // Create a new section to represent this hunk - curSection = &DiffSection{file: curFile} + curSection = newDiffSectionForDiffFile(curFile) curFile.Sections = append(curFile.Sections, curSection) lastLeftIdx = -1 } @@ -1037,7 +1060,7 @@ func parseHunks(ctx context.Context, curFile *DiffFile, maxLines, maxLineCharact } if curSection == nil { // Create a new section to represent this hunk - curSection = &DiffSection{file: curFile} + curSection = newDiffSectionForDiffFile(curFile) curFile.Sections = append(curFile.Sections, curSection) lastLeftIdx = -1 } @@ -1064,7 +1087,7 @@ func parseHunks(ctx context.Context, curFile *DiffFile, maxLines, maxLineCharact lastLeftIdx = -1 if curSection == nil { // Create a new section to represent this hunk - curSection = &DiffSection{file: curFile} + curSection = newDiffSectionForDiffFile(curFile) curFile.Sections = append(curFile.Sections, curSection) } curSection.Lines = append(curSection.Lines, diffLine) @@ -1309,7 +1332,7 @@ func GetDiffForRender(ctx context.Context, repoLink string, gitRepo *git.Reposit isVendored, isGenerated = attrs.GetVendored(), attrs.GetGenerated() language := attrs.GetLanguage() if language.Has() { - diffFile.Language = language.Value() + diffFile.language.value = language.Value() } attrDiff = attrs.Get(attribute.Diff).ToString() } @@ -1335,11 +1358,11 @@ func GetDiffForRender(ctx context.Context, repoLink string, gitRepo *git.Reposit shouldFullFileHighlight := !setting.Git.DisableDiffHighlight && attrDiff.Value() == "" if shouldFullFileHighlight { - if limitedContent.LeftContent != nil && limitedContent.LeftContent.buf.Len() < MaxDiffHighlightEntireFileSize { - diffFile.highlightedLeftLines = highlightCodeLines(diffFile, true /* left */, limitedContent.LeftContent.buf.Bytes()) + if limitedContent.LeftContent != nil { + diffFile.highlightedLeftLines.value = highlightCodeLinesForDiffFile(diffFile, true /* left */, limitedContent.LeftContent.buf.Bytes()) } - if limitedContent.RightContent != nil && limitedContent.RightContent.buf.Len() < MaxDiffHighlightEntireFileSize { - diffFile.highlightedRightLines = highlightCodeLines(diffFile, false /* right */, limitedContent.RightContent.buf.Bytes()) + if limitedContent.RightContent != nil { + diffFile.highlightedRightLines.value = highlightCodeLinesForDiffFile(diffFile, false /* right */, limitedContent.RightContent.buf.Bytes()) } } } @@ -1347,13 +1370,26 @@ func GetDiffForRender(ctx context.Context, repoLink string, gitRepo *git.Reposit return diff, nil } -func highlightCodeLines(diffFile *DiffFile, isLeft bool, rawContent []byte) map[int]template.HTML { +func FillDiffFileHighlightLinesByContent(diffFile *DiffFile, left, right []byte) { + diffFile.highlightedLeftLines.value = highlightCodeLinesForDiffFile(diffFile, true /* left */, left) + diffFile.highlightedRightLines.value = highlightCodeLinesForDiffFile(diffFile, false /* right */, right) +} + +func highlightCodeLinesForDiffFile(diffFile *DiffFile, isLeft bool, rawContent []byte) map[int]template.HTML { + return highlightCodeLines(diffFile.Name, diffFile.language.value, diffFile.Sections, isLeft, rawContent) +} + +func highlightCodeLines(name, lang string, sections []*DiffSection, isLeft bool, rawContent []byte) map[int]template.HTML { + if setting.Git.DisableDiffHighlight || len(rawContent) > MaxDiffHighlightEntireFileSize { + return nil + } + content := util.UnsafeBytesToString(charset.ToUTF8(rawContent, charset.ConvertOpts{})) - highlightedNewContent, _ := highlight.RenderCodeFast(diffFile.Name, diffFile.Language, content) + highlightedNewContent, _ := highlight.RenderCodeFast(name, lang, content) unsafeLines := highlight.UnsafeSplitHighlightedLines(highlightedNewContent) lines := make(map[int]template.HTML, len(unsafeLines)) // only save the highlighted lines we need, but not the whole file, to save memory - for _, sec := range diffFile.Sections { + for _, sec := range sections { for _, ln := range sec.Lines { lineIdx := ln.LeftIdx if !isLeft { diff --git a/services/gitdiff/gitdiff_excerpt.go b/services/gitdiff/gitdiff_excerpt.go new file mode 100644 index 0000000000..be66d8e2af --- /dev/null +++ b/services/gitdiff/gitdiff_excerpt.go @@ -0,0 +1,121 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package gitdiff + +import ( + "bufio" + "bytes" + "fmt" + "html/template" + "io" + + "code.gitea.io/gitea/modules/setting" +) + +type BlobExcerptOptions struct { + LastLeft int + LastRight int + LeftIndex int + RightIndex int + LeftHunkSize int + RightHunkSize int + Direction string + Language string +} + +func fillExcerptLines(section *DiffSection, filePath string, reader io.Reader, lang string, idxLeft, idxRight, chunkSize int) error { + buf := &bytes.Buffer{} + scanner := bufio.NewScanner(reader) + var diffLines []*DiffLine + for line := 0; line < idxRight+chunkSize; line++ { + if ok := scanner.Scan(); !ok { + break + } + lineText := scanner.Text() + if buf.Len()+len(lineText) < int(setting.UI.MaxDisplayFileSize) { + buf.WriteString(lineText) + buf.WriteByte('\n') + } + if line < idxRight { + continue + } + diffLine := &DiffLine{ + LeftIdx: idxLeft + (line - idxRight) + 1, + RightIdx: line + 1, + Type: DiffLinePlain, + Content: " " + lineText, + } + diffLines = append(diffLines, diffLine) + } + if err := scanner.Err(); err != nil { + return fmt.Errorf("fillExcerptLines scan: %w", err) + } + section.Lines = diffLines + // DiffLinePlain always uses right lines + section.highlightedRightLines.value = highlightCodeLines(filePath, lang, []*DiffSection{section}, false /* right */, buf.Bytes()) + return nil +} + +func BuildBlobExcerptDiffSection(filePath string, reader io.Reader, opts BlobExcerptOptions) (*DiffSection, error) { + lastLeft, lastRight, idxLeft, idxRight := opts.LastLeft, opts.LastRight, opts.LeftIndex, opts.RightIndex + leftHunkSize, rightHunkSize, direction := opts.LeftHunkSize, opts.RightHunkSize, opts.Direction + language := opts.Language + + chunkSize := BlobExcerptChunkSize + section := &DiffSection{ + language: &diffVarMutable[string]{value: language}, + highlightedLeftLines: &diffVarMutable[map[int]template.HTML]{}, + highlightedRightLines: &diffVarMutable[map[int]template.HTML]{}, + FileName: filePath, + } + var err error + if direction == "up" && (idxLeft-lastLeft) > chunkSize { + idxLeft -= chunkSize + idxRight -= chunkSize + leftHunkSize += chunkSize + rightHunkSize += chunkSize + err = fillExcerptLines(section, filePath, reader, language, idxLeft-1, idxRight-1, chunkSize) + } else if direction == "down" && (idxLeft-lastLeft) > chunkSize { + err = fillExcerptLines(section, filePath, reader, language, lastLeft, lastRight, chunkSize) + lastLeft += chunkSize + lastRight += chunkSize + } else { + offset := -1 + if direction == "down" { + offset = 0 + } + err = fillExcerptLines(section, filePath, reader, language, lastLeft, lastRight, idxRight-lastRight+offset) + leftHunkSize = 0 + rightHunkSize = 0 + idxLeft = lastLeft + idxRight = lastRight + } + if err != nil { + return nil, err + } + + newLineSection := &DiffLine{ + Type: DiffLineSection, + SectionInfo: &DiffLineSectionInfo{ + language: &diffVarMutable[string]{value: opts.Language}, + Path: filePath, + LastLeftIdx: lastLeft, + LastRightIdx: lastRight, + LeftIdx: idxLeft, + RightIdx: idxRight, + LeftHunkSize: leftHunkSize, + RightHunkSize: rightHunkSize, + }, + } + if newLineSection.GetExpandDirection() != "" { + newLineSection.Content = fmt.Sprintf("@@ -%d,%d +%d,%d @@\n", idxLeft, leftHunkSize, idxRight, rightHunkSize) + switch direction { + case "up": + section.Lines = append([]*DiffLine{newLineSection}, section.Lines...) + case "down": + section.Lines = append(section.Lines, newLineSection) + } + } + return section, nil +} diff --git a/services/gitdiff/gitdiff_excerpt_test.go b/services/gitdiff/gitdiff_excerpt_test.go new file mode 100644 index 0000000000..cb71e66462 --- /dev/null +++ b/services/gitdiff/gitdiff_excerpt_test.go @@ -0,0 +1,39 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package gitdiff + +import ( + "bytes" + "strconv" + "testing" + + "code.gitea.io/gitea/modules/translation" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestBuildBlobExcerptDiffSection(t *testing.T) { + data := &bytes.Buffer{} + for i := range 100 { + data.WriteString("a = " + strconv.Itoa(i+1) + "\n") + } + + locale := translation.MockLocale{} + lineMiddle := 50 + diffSection, err := BuildBlobExcerptDiffSection("a.py", bytes.NewReader(data.Bytes()), BlobExcerptOptions{ + LeftIndex: lineMiddle, + RightIndex: lineMiddle, + LeftHunkSize: 10, + RightHunkSize: 10, + Direction: "up", + }) + require.NoError(t, err) + assert.Len(t, diffSection.highlightedRightLines.value, BlobExcerptChunkSize) + assert.NotEmpty(t, diffSection.highlightedRightLines.value[lineMiddle-BlobExcerptChunkSize-1]) + assert.NotEmpty(t, diffSection.highlightedRightLines.value[lineMiddle-2]) // 0-based + + diffInline := diffSection.GetComputedInlineDiffFor(diffSection.Lines[1], locale) + assert.Equal(t, `a = 30`+"\n", string(diffInline.Content)) +} diff --git a/services/gitdiff/gitdiff_test.go b/services/gitdiff/gitdiff_test.go index a94dad8b63..62b17c223c 100644 --- a/services/gitdiff/gitdiff_test.go +++ b/services/gitdiff/gitdiff_test.go @@ -1111,22 +1111,20 @@ func TestDiffLine_GetExpandDirection(t *testing.T) { func TestHighlightCodeLines(t *testing.T) { t.Run("CharsetDetecting", func(t *testing.T) { diffFile := &DiffFile{ - Name: "a.c", - Language: "c", + Name: "a.c", Sections: []*DiffSection{ { Lines: []*DiffLine{{LeftIdx: 1}}, }, }, } - ret := highlightCodeLines(diffFile, true, []byte("// abc\xcc def\xcd")) // ISO-8859-1 bytes + ret := highlightCodeLinesForDiffFile(diffFile, true, []byte("// abc\xcc def\xcd")) // ISO-8859-1 bytes assert.Equal(t, "// abcÌ defÍ\n", string(ret[0])) }) t.Run("LeftLines", func(t *testing.T) { diffFile := &DiffFile{ - Name: "a.c", - Language: "c", + Name: "a.c", Sections: []*DiffSection{ { Lines: []*DiffLine{ @@ -1138,7 +1136,7 @@ func TestHighlightCodeLines(t *testing.T) { }, } const nl = "\n" - ret := highlightCodeLines(diffFile, true, []byte("a\nb\n")) + ret := highlightCodeLinesForDiffFile(diffFile, true, []byte("a\nb\n")) assert.Equal(t, map[int]template.HTML{ 0: `a` + nl, 1: `b`, diff --git a/services/gitdiff/highlightdiff.go b/services/gitdiff/highlightdiff.go index 11d5e6dd5a..af8215de5a 100644 --- a/services/gitdiff/highlightdiff.go +++ b/services/gitdiff/highlightdiff.go @@ -23,7 +23,7 @@ func extractDiffTokenRemainingFullTag(s string) (token, after string, valid bool // keep in mind: even if we'd like to relax this check, // we should never ignore "&" because it is for HTML entity and can't be safely used in the diff algorithm, // because diff between "<" and ">" will generate broken result. - isSymbolChar := 'a' <= c && c <= 'z' || 'A' <= c && c <= 'Z' || '0' <= c && c <= '9' || c == '_' || c == '-' + isSymbolChar := 'a' <= c && c <= 'z' || 'A' <= c && c <= 'Z' || '0' <= c && c <= '9' || c == '_' || c == '-' || c == '.' if !isSymbolChar { return "", s, false } @@ -40,7 +40,7 @@ func extractDiffTokenRemainingFullTag(s string) (token, after string, valid bool // Returned token: // * full tag with content: "<content>", it is used to optimize diff results to highlight the whole changed symbol -// * opening/close tag: "" or "" +// * opening/closing tag: "" or "" // * HTML entity: "<" func extractDiffToken(s string) (before, token, after string, valid bool) { for pos1 := 0; pos1 < len(s); pos1++ { @@ -123,6 +123,25 @@ func (hcd *highlightCodeDiff) collectUsedRunes(code template.HTML) { } } +func (hcd *highlightCodeDiff) diffEqualPartIsSpaceOnly(s string) bool { + for _, r := range s { + if r >= hcd.placeholderBegin { + recovered := hcd.placeholderTokenMap[r] + if strings.HasPrefix(recovered, "<<") { + return false // a full tag with content, it can't be space-only + } else if strings.HasPrefix(recovered, "<") { + continue // a single opening/closing tag, skip the tag and continue to check the content + } + return false // otherwise, it must be an HTML entity, it can't be space-only + } + isSpace := r == ' ' || r == '\t' || r == '\n' || r == '\r' + if !isSpace { + return false + } + } + return true +} + func (hcd *highlightCodeDiff) diffLineWithHighlight(lineType DiffLineType, codeA, codeB template.HTML) template.HTML { hcd.collectUsedRunes(codeA) hcd.collectUsedRunes(codeB) @@ -142,7 +161,21 @@ func (hcd *highlightCodeDiff) diffLineWithHighlight(lineType DiffLineType, codeA removedCodePrefix := hcd.registerTokenAsPlaceholder(``) removedCodeSuffix := hcd.registerTokenAsPlaceholder(``) - if removedCodeSuffix != 0 { + equalPartSpaceOnly := true + for _, diff := range diffs { + if diff.Type != diffmatchpatch.DiffEqual { + continue + } + if equalPartSpaceOnly = hcd.diffEqualPartIsSpaceOnly(diff.Text); !equalPartSpaceOnly { + break + } + } + + // only add "added"/"removed" tags when needed: + // * non-space contents appear in the DiffEqual parts (not a full-line add/del) + // * placeholder map still works (not exhausted, can get removedCodeSuffix) + addDiffTags := !equalPartSpaceOnly && removedCodeSuffix != 0 + if addDiffTags { for _, diff := range diffs { switch { case diff.Type == diffmatchpatch.DiffEqual: @@ -158,7 +191,7 @@ func (hcd *highlightCodeDiff) diffLineWithHighlight(lineType DiffLineType, codeA } } } else { - // placeholder map space is exhausted + // the caller will still add added/removed backgrounds for the whole line for _, diff := range diffs { take := diff.Type == diffmatchpatch.DiffEqual || (diff.Type == diffmatchpatch.DiffInsert && lineType == DiffLineAdd) || (diff.Type == diffmatchpatch.DiffDelete && lineType == DiffLineDel) if take { @@ -186,14 +219,7 @@ func (hcd *highlightCodeDiff) convertToPlaceholders(htmlContent template.HTML) s var tagStack []string res := strings.Builder{} - htmlCode := strings.TrimSpace(string(htmlContent)) - - // the standard chroma highlight HTML is ` ... ` - // the line wrapper tags should be removed before diff - if strings.HasPrefix(htmlCode, `") - } - + htmlCode := string(htmlContent) var beforeToken, token string var valid bool for { @@ -204,10 +230,16 @@ func (hcd *highlightCodeDiff) convertToPlaceholders(htmlContent template.HTML) s // write the content before the token into result string, and consume the token in the string res.WriteString(beforeToken) + // the standard chroma highlight HTML is ` ... ` + // the line wrapper tags should be removed before diff + if strings.HasPrefix(token, `" for "" diff --git a/services/gitdiff/highlightdiff_test.go b/services/gitdiff/highlightdiff_test.go index 573177f401..89e320f2c0 100644 --- a/services/gitdiff/highlightdiff_test.go +++ b/services/gitdiff/highlightdiff_test.go @@ -14,25 +14,57 @@ import ( "github.com/stretchr/testify/assert" ) -func TestDiffWithHighlight(t *testing.T) { - t.Run("DiffLineAddDel", func(t *testing.T) { +func BenchmarkHighlightDiff(b *testing.B) { + for b.Loop() { + // still fast enough: BenchmarkHighlightDiff-12 1000000 1027 ns/op + // TODO: the real bottleneck is that "diffLineWithHighlight" is called twice when rendering "added" and "removed" lines by the caller + // Ideally the caller should cache the diff result, and then use the diff result to render "added" and "removed" lines separately hcd := newHighlightCodeDiff() codeA := template.HTML(`x foo y`) codeB := template.HTML(`x bar y`) - outDel := hcd.diffLineWithHighlight(DiffLineDel, codeA, codeB) - assert.Equal(t, `x foo y`, string(outDel)) - outAdd := hcd.diffLineWithHighlight(DiffLineAdd, codeA, codeB) - assert.Equal(t, `x bar y`, string(outAdd)) + hcd.diffLineWithHighlight(DiffLineDel, codeA, codeB) + } +} + +func TestDiffWithHighlight(t *testing.T) { + t.Run("DiffLineAddDel", func(t *testing.T) { + t.Run("WithDiffTags", func(t *testing.T) { + hcd := newHighlightCodeDiff() + codeA := template.HTML(`x foo y`) + codeB := template.HTML(`x bar y`) + outDel := hcd.diffLineWithHighlight(DiffLineDel, codeA, codeB) + assert.Equal(t, `x foo y`, string(outDel)) + outAdd := hcd.diffLineWithHighlight(DiffLineAdd, codeA, codeB) + assert.Equal(t, `x bar y`, string(outAdd)) + }) + t.Run("NoRedundantTags", func(t *testing.T) { + // the equal parts only contain spaces, in this case, don't use "added/removed" tags + // because the diff lines already have a background color to indicate the change + hcd := newHighlightCodeDiff() + codeA := template.HTML(" \tfoo ") + codeB := template.HTML(" bar \n") + outDel := hcd.diffLineWithHighlight(DiffLineDel, codeA, codeB) + assert.Equal(t, string(codeA), string(outDel)) + outAdd := hcd.diffLineWithHighlight(DiffLineAdd, codeA, codeB) + assert.Equal(t, string(codeB), string(outAdd)) + }) }) t.Run("CleanUp", func(t *testing.T) { hcd := newHighlightCodeDiff() - codeA := template.HTML(`this is a comment`) - codeB := template.HTML(`this is updated comment`) + codeA := template.HTML(` this is a comment`) + codeB := template.HTML(` this is updated comment`) outDel := hcd.diffLineWithHighlight(DiffLineDel, codeA, codeB) - assert.Equal(t, `this is a comment`, string(outDel)) + assert.Equal(t, ` this is a comment`, string(outDel)) outAdd := hcd.diffLineWithHighlight(DiffLineAdd, codeA, codeB) - assert.Equal(t, `this is updated comment`, string(outAdd)) + assert.Equal(t, ` this is updated comment`, string(outAdd)) + + codeA = `line1` + "\n" + `line2` + codeB = `line1` + "\n" + `line!` + outDel = hcd.diffLineWithHighlight(DiffLineDel, codeA, codeB) + assert.Equal(t, `line1`+"\n"+`line2`, string(outDel)) + outAdd = hcd.diffLineWithHighlight(DiffLineAdd, codeA, codeB) + assert.Equal(t, `line1`+"\n"+`line!`, string(outAdd)) }) t.Run("OpenCloseTags", func(t *testing.T) { diff --git a/services/repository/files/diff.go b/services/repository/files/diff.go index 50d01f9d7c..aa4b55a307 100644 --- a/services/repository/files/diff.go +++ b/services/repository/files/diff.go @@ -12,7 +12,7 @@ import ( ) // GetDiffPreview produces and returns diff result of a file which is not yet committed. -func GetDiffPreview(ctx context.Context, repo *repo_model.Repository, branch, treePath, content string) (*gitdiff.Diff, error) { +func GetDiffPreview(ctx context.Context, repo *repo_model.Repository, branch, treePath, oldContent, newContent string) (*gitdiff.Diff, error) { if branch == "" { branch = repo.DefaultBranch } @@ -29,7 +29,7 @@ func GetDiffPreview(ctx context.Context, repo *repo_model.Repository, branch, tr } // Add the object to the database - objectHash, err := t.HashObjectAndWrite(ctx, strings.NewReader(content)) + objectHash, err := t.HashObjectAndWrite(ctx, strings.NewReader(newContent)) if err != nil { return nil, err } @@ -38,5 +38,5 @@ func GetDiffPreview(ctx context.Context, repo *repo_model.Repository, branch, tr if err := t.AddObjectToIndex(ctx, "100644", objectHash, treePath); err != nil { return nil, err } - return t.DiffIndex(ctx) + return t.DiffIndex(ctx, oldContent, newContent) } diff --git a/services/repository/files/diff_test.go b/services/repository/files/diff_test.go index ae702e4189..5295879621 100644 --- a/services/repository/files/diff_test.go +++ b/services/repository/files/diff_test.go @@ -27,8 +27,30 @@ func TestGetDiffPreview(t *testing.T) { branch := ctx.Repo.Repository.DefaultBranch treePath := "README.md" + oldContent := "# repo1\n\nDescription for repo1" content := "# repo1\n\nDescription for repo1\nthis is a new line" + t.Run("Errors", func(t *testing.T) { + t.Run("empty repo", func(t *testing.T) { + diff, err := GetDiffPreview(ctx, &repo_model.Repository{}, branch, treePath, oldContent, content) + assert.Nil(t, diff) + assert.EqualError(t, err, "repository does not exist [id: 0, uid: 0, owner_name: , name: ]") + }) + + t.Run("bad branch", func(t *testing.T) { + badBranch := "bad_branch" + diff, err := GetDiffPreview(ctx, ctx.Repo.Repository, badBranch, treePath, oldContent, content) + assert.Nil(t, diff) + assert.EqualError(t, err, "branch does not exist [name: "+badBranch+"]") + }) + + t.Run("empty treePath", func(t *testing.T) { + diff, err := GetDiffPreview(ctx, ctx.Repo.Repository, branch, "", oldContent, content) + assert.Nil(t, diff) + assert.EqualError(t, err, "path is invalid [path: ]") + }) + }) + expectedDiff := &gitdiff.Diff{ Files: []*gitdiff.DiffFile{ { @@ -112,56 +134,22 @@ func TestGetDiffPreview(t *testing.T) { } t.Run("with given branch", func(t *testing.T) { - diff, err := GetDiffPreview(ctx, ctx.Repo.Repository, branch, treePath, content) + diff, err := GetDiffPreview(ctx, ctx.Repo.Repository, branch, treePath, oldContent, content) assert.NoError(t, err) expectedBs, err := json.Marshal(expectedDiff) assert.NoError(t, err) bs, err := json.Marshal(diff) assert.NoError(t, err) - assert.Equal(t, string(expectedBs), string(bs)) + assert.JSONEq(t, string(expectedBs), string(bs)) }) t.Run("empty branch, same results", func(t *testing.T) { - diff, err := GetDiffPreview(ctx, ctx.Repo.Repository, "", treePath, content) + diff, err := GetDiffPreview(ctx, ctx.Repo.Repository, "", treePath, oldContent, content) assert.NoError(t, err) expectedBs, err := json.Marshal(expectedDiff) assert.NoError(t, err) bs, err := json.Marshal(diff) assert.NoError(t, err) - assert.Equal(t, expectedBs, bs) - }) -} - -func TestGetDiffPreviewErrors(t *testing.T) { - unittest.PrepareTestEnv(t) - ctx, _ := contexttest.MockContext(t, "user2/repo1") - ctx.SetPathParam("id", "1") - contexttest.LoadRepo(t, ctx, 1) - contexttest.LoadRepoCommit(t, ctx) - contexttest.LoadUser(t, ctx, 2) - contexttest.LoadGitRepo(t, ctx) - defer ctx.Repo.GitRepo.Close() - - branch := ctx.Repo.Repository.DefaultBranch - treePath := "README.md" - content := "# repo1\n\nDescription for repo1\nthis is a new line" - - t.Run("empty repo", func(t *testing.T) { - diff, err := GetDiffPreview(ctx, &repo_model.Repository{}, branch, treePath, content) - assert.Nil(t, diff) - assert.EqualError(t, err, "repository does not exist [id: 0, uid: 0, owner_name: , name: ]") - }) - - t.Run("bad branch", func(t *testing.T) { - badBranch := "bad_branch" - diff, err := GetDiffPreview(ctx, ctx.Repo.Repository, badBranch, treePath, content) - assert.Nil(t, diff) - assert.EqualError(t, err, "branch does not exist [name: "+badBranch+"]") - }) - - t.Run("empty treePath", func(t *testing.T) { - diff, err := GetDiffPreview(ctx, ctx.Repo.Repository, branch, "", content) - assert.Nil(t, diff) - assert.EqualError(t, err, "path is invalid [path: ]") + assert.JSONEq(t, string(expectedBs), string(bs)) }) } diff --git a/services/repository/files/temp_repo.go b/services/repository/files/temp_repo.go index 63f4f06d25..a579b807ba 100644 --- a/services/repository/files/temp_repo.go +++ b/services/repository/files/temp_repo.go @@ -361,7 +361,7 @@ func (t *TemporaryUploadRepository) Push(ctx context.Context, doer *user_model.U } // DiffIndex returns a Diff of the current index to the head -func (t *TemporaryUploadRepository) DiffIndex(ctx context.Context) (*gitdiff.Diff, error) { +func (t *TemporaryUploadRepository) DiffIndex(ctx context.Context, oldContent, newContent string) (*gitdiff.Diff, error) { var diff *gitdiff.Diff cmd := gitcmd.NewCommand("diff-index", "--src-prefix=\\a/", "--dst-prefix=\\b/", "--cached", "-p", "HEAD") stdoutReader, stdoutReaderClose := cmd.MakeStdoutPipe() @@ -383,6 +383,9 @@ func (t *TemporaryUploadRepository) DiffIndex(ctx context.Context) (*gitdiff.Dif return nil, fmt.Errorf("unable to run diff-index pipeline in temporary repo: %w", err) } + if len(diff.Files) > 0 { + gitdiff.FillDiffFileHighlightLinesByContent(diff.Files[0], util.UnsafeStringToBytes(oldContent), util.UnsafeStringToBytes(newContent)) + } return diff, nil } diff --git a/tests/integration/editor_test.go b/tests/integration/editor_test.go index d4681f1699..c70bb061c9 100644 --- a/tests/integration/editor_test.go +++ b/tests/integration/editor_test.go @@ -148,10 +148,10 @@ func testEditFileToNewBranch(t *testing.T, session *TestSession, user, repo, bra func testEditorDiffPreview(t *testing.T) { session := loginUser(t, "user2") req := NewRequestWithValues(t, "POST", "/user2/repo1/_preview/master/README.md", map[string]string{ - "content": "Hello, World (Edited)\n", + "content": "# repo1 (Edited)", }) resp := session.MakeRequest(t, req, http.StatusOK) - assert.Contains(t, resp.Body.String(), `Hello, World (Edited)`) + assert.Contains(t, resp.Body.String(), ` (Edited)`) } func testEditorPatchFile(t *testing.T) { diff --git a/tests/integration/pull_commit_test.go b/tests/integration/pull_commit_test.go index 9f3b1a9ef5..01b8ec1ff4 100644 --- a/tests/integration/pull_commit_test.go +++ b/tests/integration/pull_commit_test.go @@ -36,6 +36,6 @@ func TestListPullCommits(t *testing.T) { defer tests.PrintCurrentTest(t)() req = NewRequest(t, "GET", "/user2/repo1/blob_excerpt/985f0301dba5e7b34be866819cd15ad3d8f508ee?last_left=0&last_right=0&left=2&right=2&left_hunk_size=2&right_hunk_size=2&path=README.md&style=split&direction=up") resp = session.MakeRequest(t, req, http.StatusOK) - assert.Contains(t, resp.Body.String(), `# repo1`) + assert.Contains(t, resp.Body.String(), `# repo1`+"\n"+``) }) }