From 290568a07b25552d3656ed5e3c154d6c3ca58263 Mon Sep 17 00:00:00 2001 From: Giteabot Date: Tue, 20 Oct 2026 20:37:19 -0700 Subject: [PATCH] fix: off-by-one errors in code comment patches (#39887) (#39904) Backport #39887 by @silverwind Code comments next to a diff hunk could show one line too high in the conversation view vs. in the code view that showed them on the correct line. - Treat the hunk end in `CutDiffAroundLine` as exclusive, so the line after a hunk falls back to the unchanged-line context - Don't count `\ No newline at end of file` markers as lines when cutting the hunk - Share the unchanged-line fallback with migrated review comments Co-authored-by: silverwind --- modules/git/diff.go | 30 ++++++++++++--------------- modules/git/diff_test.go | 10 ++++++++- services/gitdiff/gitdiff.go | 16 ++++++++++++++ services/migrations/gitea_uploader.go | 6 ++---- services/pull/review.go | 16 +------------- 5 files changed, 41 insertions(+), 37 deletions(-) diff --git a/modules/git/diff.go b/modules/git/diff.go index 8f49fb837a0..5326d4024a8 100644 --- a/modules/git/diff.go +++ b/modules/git/diff.go @@ -5,6 +5,7 @@ package git import ( "bufio" + "cmp" "context" "fmt" "io" @@ -200,22 +201,18 @@ func CutDiffAroundLine(originalDiff io.Reader, line int64, old bool, numbersOfLi } if old { begin, _ = strconv.ParseInt(groups["beginOld"], 10, 64) - end, _ = strconv.ParseInt(groups["endOld"], 10, 64) + end, _ = strconv.ParseInt(cmp.Or(groups["endOld"], "1"), 10, 64) // init otherLine with begin of opposite side otherLine, _ = strconv.ParseInt(groups["beginNew"], 10, 64) } else { begin, _ = strconv.ParseInt(groups["beginNew"], 10, 64) - if groups["endNew"] != "" { - end, _ = strconv.ParseInt(groups["endNew"], 10, 64) - } else { - end = 0 - } + end, _ = strconv.ParseInt(cmp.Or(groups["endNew"], "1"), 10, 64) // init otherLine with begin of opposite side otherLine, _ = strconv.ParseInt(groups["beginOld"], 10, 64) } end += begin // end is for real only the number of lines in hunk // lof is between begin and end - if begin <= line && end >= line { + if begin <= line && line < end { hunk = append(hunk, lof) currentLine = begin continue @@ -265,27 +262,26 @@ func CutDiffAroundLine(originalDiff io.Reader, line int64, old bool, numbersOfLi oldBegin = otherLine newBegin = currentLine } - // headers + hunk header - newHunk := make([]string, headerLines) - // transfer existing headers - copy(newHunk, hunk[:headerLines]) - // transfer last n lines - newHunk = append(newHunk, hunk[len(hunk)-numbersOfLine-1:]...) // calculate newBegin, ... by counting lines - for i := len(hunk) - 1; i >= len(hunk)-numbersOfLine; i-- { - switch hunk[i][0] { + start := len(hunk) + for count := 0; count < numbersOfLine && start > headerLines+1; { + start-- + switch hunk[start][0] { case '+': newBegin-- newNumOfLines++ case '-': oldBegin-- oldNumOfLines++ + case '\\': + continue default: oldBegin-- newBegin-- newNumOfLines++ oldNumOfLines++ } + count++ } // "git diff" outputs "@@ -1 +1,3 @@" for "OLD" => "A\nB\nC" @@ -294,9 +290,9 @@ func CutDiffAroundLine(originalDiff io.Reader, line int64, old bool, numbersOfLi // For example: "L1\nL2" => "A\nB", then the patch shows "L2" as line 1 on the left (deleted part) // construct the new hunk header - newHunk[headerLines] = fmt.Sprintf("@@ -%d,%d +%d,%d @@", + hunk[headerLines] = fmt.Sprintf("@@ -%d,%d +%d,%d @@", oldBegin, oldNumOfLines, newBegin, newNumOfLines) - return strings.Join(newHunk, "\n"), nil + return strings.Join(append(hunk[:headerLines+1], hunk[start:]...), "\n"), nil } // GetAffectedFiles returns the affected files between two commits diff --git a/modules/git/diff_test.go b/modules/git/diff_test.go index 7671fffcc16..454405fdcd6 100644 --- a/modules/git/diff_test.go +++ b/modules/git/diff_test.go @@ -120,10 +120,18 @@ func TestCutDiffAroundLine(t *testing.T) { assert.Empty(t, emptyResult) // Line is out of scope - emptyResult, err = CutDiffAroundLine(strings.NewReader(exampleDiff), 434, false, 0) + emptyResult, err = CutDiffAroundLine(strings.NewReader(exampleDiff), 7, false, 3) assert.NoError(t, err) assert.Empty(t, emptyResult) + noNewlineDiff := "diff --git a/a b/a\n--- a/a\n+++ b/a\n@@ -1 +1 @@\n-a\n\\ No newline at end of file\n+a" + result, err = CutDiffAroundLine(strings.NewReader(noNewlineDiff), 1, false, 2) + assert.NoError(t, err) + assert.Equal(t, "diff --git a/a b/a\n--- a/a\n+++ b/a\n@@ -1,1 +1,1 @@\n-a\n\\ No newline at end of file\n+a", result) + result, err = CutDiffAroundLine(strings.NewReader(noNewlineDiff), 1, true, 2) + assert.NoError(t, err) + assert.Equal(t, "diff --git a/a b/a\n--- a/a\n+++ b/a\n@@ -1 +1 @@\n-a", result) + // Handle minus diffs properly minusDiff, err := CutDiffAroundLine(strings.NewReader(breakingDiff), 2, false, 4) assert.NoError(t, err) diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index bef5ed22a79..f8630e656bb 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -1620,6 +1620,22 @@ func CommentAsDiff(ctx context.Context, c *issues_model.Comment) (*Diff, error) return diff, nil } +// GetCodeCommentPatch returns the code snippet stored with a code comment +func GetCodeCommentPatch(ctx context.Context, gitRepo *git.Repository, mergeBase, headCommitID, treePath string, line int64) (string, error) { + patch, err := git.GetFileDiffCutAroundLine(ctx, gitRepo, mergeBase, headCommitID, treePath, + int64((&issues_model.Comment{Line: line}).UnsignedLine()), line < 0, setting.UI.CodeCommentLines, + ) + contextCommitID := util.Iif(line < 0, mergeBase, headCommitID) + if err != nil || patch != "" || contextCommitID == "" { + return patch, err + } + patch, err = GeneratePatchForUnchangedLine(ctx, gitRepo, contextCommitID, treePath, line, setting.UI.CodeCommentLines) + if err != nil { + log.Debug("Unable to generate patch for unchanged line (file=%s, line=%d, commit=%s): %v", treePath, line, contextCommitID, err) + } + return patch, nil +} + // GeneratePatchForUnchangedLine creates a patch showing code context for an unchanged line func GeneratePatchForUnchangedLine(ctx context.Context, gitRepo *git.Repository, commitID, treePath string, line int64, contextLines int) (string, error) { commit, err := gitRepo.GetCommit(ctx, commitID) diff --git a/services/migrations/gitea_uploader.go b/services/migrations/gitea_uploader.go index 9463b8c2d9a..5134aa0a0a1 100644 --- a/services/migrations/gitea_uploader.go +++ b/services/migrations/gitea_uploader.go @@ -31,6 +31,7 @@ import ( "gitea.dev/modules/timeutil" "gitea.dev/modules/uri" "gitea.dev/modules/util" + "gitea.dev/services/gitdiff" "gitea.dev/services/pull" repo_service "gitea.dev/services/repository" ) @@ -905,10 +906,7 @@ func (g *GiteaLocalUploader) CreateReviews(ctx context.Context, reviews ...*base // SECURITY: The TreePath must be cleaned! use relative path comment.TreePath = util.PathJoinRel(comment.TreePath) - patch, _ := git.GetFileDiffCutAroundLine(ctx, - g.gitRepo, pr.MergeBase, headCommitID, comment.TreePath, - int64((&issues_model.Comment{Line: int64(line + comment.Position - 1)}).UnsignedLine()), line < 0, setting.UI.CodeCommentLines, - ) + patch, _ := gitdiff.GetCodeCommentPatch(ctx, g.gitRepo, pr.MergeBase, headCommitID, comment.TreePath, int64(line+comment.Position-1)) if comment.CreatedAt.IsZero() { comment.CreatedAt = review.CreatedAt diff --git a/services/pull/review.go b/services/pull/review.go index 08c7e08514e..43ddca15aab 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -17,7 +17,6 @@ import ( "gitea.dev/modules/git/gitcmd" "gitea.dev/modules/log" "gitea.dev/modules/optional" - "gitea.dev/modules/setting" "gitea.dev/modules/util" "gitea.dev/services/gitdiff" notify_service "gitea.dev/services/notify" @@ -271,23 +270,10 @@ func createCodeComment(ctx context.Context, doer *user_model.User, repo *repo_mo commitID = headCommitID } - patch, err = git.GetFileDiffCutAroundLine(ctx, - gitRepo, pr.MergeBase, headCommitID, treePath, - int64((&issues_model.Comment{Line: line}).UnsignedLine()), line < 0, setting.UI.CodeCommentLines, - ) + patch, err = gitdiff.GetCodeCommentPatch(ctx, gitRepo, pr.MergeBase, headCommitID, treePath, line) if err != nil { return nil, err } - - // If patch is still empty (unchanged line), generate code context - contextCommitID := util.Iif(line < 0, pr.MergeBase, headCommitID) // the line number is relative to the diff sides, not to the blamed commitID - if patch == "" && contextCommitID != "" { - patch, err = gitdiff.GeneratePatchForUnchangedLine(ctx, gitRepo, contextCommitID, treePath, line, setting.UI.CodeCommentLines) - if err != nil { - // Log the error but don't fail comment creation - log.Debug("Unable to generate patch for unchanged line (file=%s, line=%d, commit=%s): %v", treePath, line, contextCommitID, err) - } - } } return issues_model.CreateComment(ctx, &issues_model.CreateCommentOptions{ Type: issues_model.CommentTypeCode,