From dbb8312ff3e58af01beada1b35652ec2d9701569 Mon Sep 17 00:00:00 2001 From: silverwind Date: Fri, 20 Feb 2026 13:08:24 +0100 Subject: [PATCH] DRY blob excerpt: unified single/batch path, single-pass highlighting - Merge ExcerptBlob and excerptBlobBatch into a single unified flow that handles both single and batch requests without duplicated code - Extract parseBatchBlobExcerptOptions to DRY the repetitive splitInts calls - Refactor BuildBlobExcerptDiffSection to separate section building from highlighting, enabling BuildBlobExcerptDiffSections (plural) to highlight the file content only once for all sections instead of N times - Add blob size limit check (MaxDisplayFileSize) before io.ReadAll to prevent OOM on large files Co-Authored-By: Claude Opus 4.6 --- routers/web/repo/compare.go | 222 +++++++++++----------------- services/gitdiff/gitdiff_excerpt.go | 60 ++++++-- 2 files changed, 131 insertions(+), 151 deletions(-) diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index 9bf3175615..4ab39d80fc 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -4,7 +4,6 @@ package repo import ( - "bytes" gocontext "context" "encoding/csv" "errors" @@ -765,17 +764,8 @@ func splitInts(s string) ([]int, error) { // ExcerptBlob render blob excerpt contents func ExcerptBlob(ctx *context.Context) { commitID := ctx.PathParam("sha") - 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") + language := ctx.FormString("filelang") gitRepo := ctx.Repo.GitRepo diffBlobExcerptData := &gitdiff.DiffBlobExcerptData{ @@ -794,11 +784,28 @@ func ExcerptBlob(ctx *context.Context) { diffBlobExcerptData.BaseLink = ctx.Repo.RepoLink + "/wiki/blob_excerpt" } - // Batch mode: if last_left contains a comma, treat all per-gap params as - // comma-separated lists and return a JSON array of HTML strings. - if strings.Contains(ctx.FormString("last_left"), ",") { - excerptBlobBatch(ctx, gitRepo, commitID, filePath, opts.Language, diffBlobExcerptData) - return + // Detect batch mode: comma in last_left means comma-separated arrays + isBatch := strings.Contains(ctx.FormString("last_left"), ",") + + // Parse options: batch parses comma-separated arrays, single parses individual values + var optsList []gitdiff.BlobExcerptOptions + if isBatch { + var ok bool + optsList, ok = parseBatchBlobExcerptOptions(ctx, language) + if !ok { + return + } + } else { + optsList = []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: language, + }} } commit, err := gitRepo.GetCommit(commitID) @@ -811,26 +818,35 @@ func ExcerptBlob(ctx *context.Context) { ctx.ServerError("GetBlobByPath", err) return } + if blob.Size() > setting.UI.MaxDisplayFileSize { + ctx.HTTPError(http.StatusRequestEntityTooLarge, "blob too large for expansion") + return + } reader, err := blob.DataAsync() if err != nil { ctx.ServerError("DataAsync", err) return } - defer reader.Close() - - section, err := gitdiff.BuildBlobExcerptDiffSection(filePath, reader, opts) + blobData, err := io.ReadAll(reader) + reader.Close() if err != nil { - ctx.ServerError("BuildBlobExcerptDiffSection", err) + ctx.ServerError("ReadAll", err) return } + sections, err := gitdiff.BuildBlobExcerptDiffSections(filePath, blobData, optsList) + if err != nil { + ctx.ServerError("BuildBlobExcerptDiffSections", err) + return + } + + // Fetch PR comments and attach to sections diffBlobExcerptData.PullIssueIndex = ctx.FormInt64("pull_issue_index") if diffBlobExcerptData.PullIssueIndex > 0 { if !ctx.Repo.CanRead(unit.TypePullRequests) { ctx.NotFound(nil) return } - issue, err := issues_model.GetIssueByIndex(ctx, ctx.Repo.Repository.ID, diffBlobExcerptData.PullIssueIndex) if err != nil { log.Error("GetIssueByIndex error: %v", err) @@ -847,8 +863,8 @@ func ExcerptBlob(ctx *context.Context) { allComments, err := issues_model.FetchCodeComments(ctx, issue, ctx.Doer, ctx.FormBool("show_outdated")) if err != nil { log.Error("FetchCodeComments error: %v", err) - } else { - if lineComments, ok := allComments[filePath]; ok { + } else if lineComments, ok := allComments[filePath]; ok { + for _, section := range sections { attachCommentsToLines(section, lineComments) attachHiddenCommentIDs(section, lineComments) } @@ -856,134 +872,62 @@ func ExcerptBlob(ctx *context.Context) { } } - ctx.Data["section"] = section ctx.Data["FileNameHash"] = git.HashFilePathForWebUI(filePath) ctx.Data["DiffBlobExcerptData"] = diffBlobExcerptData - ctx.HTML(http.StatusOK, tplBlobExcerpt) + // Respond: single returns HTML, batch returns JSON array of HTML strings + if isBatch { + htmlStrings := make([]string, len(sections)) + for i, section := range sections { + ctx.Data["section"] = section + html, err := ctx.RenderToHTML(tplBlobExcerpt, ctx.Data) + if err != nil { + ctx.ServerError("RenderToHTML", err) + return + } + htmlStrings[i] = string(html) + } + ctx.JSON(http.StatusOK, htmlStrings) + } else { + ctx.Data["section"] = sections[0] + ctx.HTML(http.StatusOK, tplBlobExcerpt) + } } -func excerptBlobBatch(ctx *context.Context, gitRepo *git.Repository, commitID, filePath, language string, diffBlobExcerptData *gitdiff.DiffBlobExcerptData) { - lastLefts, err := splitInts(ctx.FormString("last_left")) - if err != nil { - ctx.HTTPError(http.StatusBadRequest, "invalid last_left values") - return - } - lastRights, err := splitInts(ctx.FormString("last_right")) - if err != nil { - ctx.HTTPError(http.StatusBadRequest, "invalid last_right values") - return - } - lefts, err := splitInts(ctx.FormString("left")) - if err != nil { - ctx.HTTPError(http.StatusBadRequest, "invalid left values") - return - } - rights, err := splitInts(ctx.FormString("right")) - if err != nil { - ctx.HTTPError(http.StatusBadRequest, "invalid right values") - return - } - leftHunkSizes, err := splitInts(ctx.FormString("left_hunk_size")) - if err != nil { - ctx.HTTPError(http.StatusBadRequest, "invalid left_hunk_size values") - return - } - rightHunkSizes, err := splitInts(ctx.FormString("right_hunk_size")) - if err != nil { - ctx.HTTPError(http.StatusBadRequest, "invalid right_hunk_size values") - return - } - - n := len(lastLefts) - if len(lastRights) != n || len(lefts) != n || len(rights) != n || len(leftHunkSizes) != n || len(rightHunkSizes) != n { - ctx.HTTPError(http.StatusBadRequest, "all per-gap parameter arrays must have the same length") - return - } - - commit, err := gitRepo.GetCommit(commitID) - if err != nil { - ctx.ServerError("GetCommit", err) - return - } - blob, err := commit.Tree.GetBlobByPath(filePath) - if err != nil { - ctx.ServerError("GetBlobByPath", err) - return - } - reader, err := blob.DataAsync() - if err != nil { - ctx.ServerError("DataAsync", err) - return - } - blobData, err := io.ReadAll(reader) - reader.Close() - if err != nil { - ctx.ServerError("ReadAll", err) - return - } - - diffBlobExcerptData.PullIssueIndex = ctx.FormInt64("pull_issue_index") - var lineComments map[int64][]*issues_model.Comment - if diffBlobExcerptData.PullIssueIndex > 0 { - if !ctx.Repo.CanRead(unit.TypePullRequests) { - ctx.NotFound(nil) - return - } - issue, err := issues_model.GetIssueByIndex(ctx, ctx.Repo.Repository.ID, diffBlobExcerptData.PullIssueIndex) +// parseBatchBlobExcerptOptions parses comma-separated per-gap parameters for batch expansion. +// Returns false if an error response has been sent. +func parseBatchBlobExcerptOptions(ctx *context.Context, language string) ([]gitdiff.BlobExcerptOptions, bool) { + paramNames := [6]string{"last_left", "last_right", "left", "right", "left_hunk_size", "right_hunk_size"} + var parsed [6][]int + for i, name := range paramNames { + vals, err := splitInts(ctx.FormString(name)) if err != nil { - log.Error("GetIssueByIndex error: %v", err) - } else if issue.IsPull { - ctx.Data["Issue"] = issue - ctx.Data["CanBlockUser"] = func(blocker, blockee *user_model.User) bool { - return user_service.CanBlockUser(ctx, ctx.Doer, blocker, blockee) - } - ctx.Data["PageIsPullFiles"] = true - ctx.Data["AfterCommitID"] = diffBlobExcerptData.AfterCommitID - allComments, err := issues_model.FetchCodeComments(ctx, issue, ctx.Doer, ctx.FormBool("show_outdated")) - if err != nil { - log.Error("FetchCodeComments error: %v", err) - } else { - lineComments = allComments[filePath] - } + ctx.HTTPError(http.StatusBadRequest, "invalid "+name+" values") + return nil, false + } + parsed[i] = vals + } + + n := len(parsed[0]) + for i := 1; i < len(parsed); i++ { + if len(parsed[i]) != n { + ctx.HTTPError(http.StatusBadRequest, "all per-gap parameter arrays must have the same length") + return nil, false } } - ctx.Data["FileNameHash"] = git.HashFilePathForWebUI(filePath) - ctx.Data["DiffBlobExcerptData"] = diffBlobExcerptData - - htmlStrings := make([]string, n) + optsList := make([]gitdiff.BlobExcerptOptions, n) for i := range n { - opts := gitdiff.BlobExcerptOptions{ - LastLeft: lastLefts[i], - LastRight: lastRights[i], - LeftIndex: lefts[i], - RightIndex: rights[i], - LeftHunkSize: leftHunkSizes[i], - RightHunkSize: rightHunkSizes[i], + optsList[i] = gitdiff.BlobExcerptOptions{ + LastLeft: parsed[0][i], + LastRight: parsed[1][i], + LeftIndex: parsed[2][i], + RightIndex: parsed[3][i], + LeftHunkSize: parsed[4][i], + RightHunkSize: parsed[5][i], Direction: "full", Language: language, } - - section, err := gitdiff.BuildBlobExcerptDiffSection(filePath, bytes.NewReader(blobData), opts) - if err != nil { - ctx.ServerError("BuildBlobExcerptDiffSection", err) - return - } - - if lineComments != nil { - attachCommentsToLines(section, lineComments) - attachHiddenCommentIDs(section, lineComments) - } - - ctx.Data["section"] = section - html, err := ctx.RenderToHTML(tplBlobExcerpt, ctx.Data) - if err != nil { - ctx.ServerError("RenderToHTML", err) - return - } - htmlStrings[i] = string(html) } - - ctx.JSON(http.StatusOK, htmlStrings) + return optsList, true } diff --git a/services/gitdiff/gitdiff_excerpt.go b/services/gitdiff/gitdiff_excerpt.go index 4b1958fc11..e373f90bbb 100644 --- a/services/gitdiff/gitdiff_excerpt.go +++ b/services/gitdiff/gitdiff_excerpt.go @@ -26,7 +26,9 @@ type BlobExcerptOptions struct { Language string } -func fillExcerptLines(section *DiffSection, filePath string, reader io.Reader, lang string, idxLeft, idxRight, chunkSize int) error { +// fillExcerptLines reads from reader and populates section.Lines. +// It returns the accumulated content buffer for later highlighting. +func fillExcerptLines(section *DiffSection, reader io.Reader, idxLeft, idxRight, chunkSize int) ([]byte, error) { buf := &bytes.Buffer{} scanner := bufio.NewScanner(reader) var diffLines []*DiffLine @@ -51,36 +53,36 @@ func fillExcerptLines(section *DiffSection, filePath string, reader io.Reader, l diffLines = append(diffLines, diffLine) } if err := scanner.Err(); err != nil { - return fmt.Errorf("fillExcerptLines scan: %w", err) + return nil, 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 + return buf.Bytes(), nil } -func BuildBlobExcerptDiffSection(filePath string, reader io.Reader, opts BlobExcerptOptions) (*DiffSection, error) { +// buildExcerptDiffSection builds a single excerpt section without highlighting. +// It returns the section and the accumulated content buffer. +func buildExcerptDiffSection(filePath string, reader io.Reader, opts BlobExcerptOptions) (*DiffSection, []byte, 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}, + language: &diffVarMutable[string]{value: opts.Language}, highlightLexer: &diffVarMutable[chroma.Lexer]{}, highlightedLeftLines: &diffVarMutable[map[int]template.HTML]{}, highlightedRightLines: &diffVarMutable[map[int]template.HTML]{}, FileName: filePath, } + var bufContent []byte 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) + bufContent, err = fillExcerptLines(section, reader, idxLeft-1, idxRight-1, chunkSize) } else if direction == "down" && (idxLeft-lastLeft) > chunkSize { - err = fillExcerptLines(section, filePath, reader, language, lastLeft, lastRight, chunkSize) + bufContent, err = fillExcerptLines(section, reader, lastLeft, lastRight, chunkSize) lastLeft += chunkSize lastRight += chunkSize } else { @@ -88,14 +90,14 @@ func BuildBlobExcerptDiffSection(filePath string, reader io.Reader, opts BlobExc if direction == "down" { offset = 0 } - err = fillExcerptLines(section, filePath, reader, language, lastLeft, lastRight, idxRight-lastRight+offset) + bufContent, err = fillExcerptLines(section, reader, lastLeft, lastRight, idxRight-lastRight+offset) leftHunkSize = 0 rightHunkSize = 0 idxLeft = lastLeft idxRight = lastRight } if err != nil { - return nil, err + return nil, nil, err } newLineSection := &DiffLine{ @@ -120,5 +122,39 @@ func BuildBlobExcerptDiffSection(filePath string, reader io.Reader, opts BlobExc section.Lines = append(section.Lines, newLineSection) } } + return section, bufContent, nil +} + +// BuildBlobExcerptDiffSection builds a single excerpt section with highlighting. +func BuildBlobExcerptDiffSection(filePath string, reader io.Reader, opts BlobExcerptOptions) (*DiffSection, error) { + section, bufContent, err := buildExcerptDiffSection(filePath, reader, opts) + if err != nil { + return nil, err + } + // DiffLinePlain always uses right lines + section.highlightedRightLines.value = highlightCodeLines(filePath, opts.Language, []*DiffSection{section}, false /* right */, bufContent) return section, nil } + +// BuildBlobExcerptDiffSections builds multiple excerpt sections from the same file content, +// highlighting the content only once for all sections. +func BuildBlobExcerptDiffSections(filePath string, content []byte, optsList []BlobExcerptOptions) ([]*DiffSection, error) { + sections := make([]*DiffSection, len(optsList)) + for i, opts := range optsList { + section, _, err := buildExcerptDiffSection(filePath, bytes.NewReader(content), opts) + if err != nil { + return nil, err + } + sections[i] = section + } + + // Highlight once for all sections + if len(optsList) > 0 { + highlighted := highlightCodeLines(filePath, optsList[0].Language, sections, false /* right */, content) + for _, section := range sections { + section.highlightedRightLines.value = highlighted + } + } + + return sections, nil +}