diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini index 19b54a5b2d..ecd64495b9 100644 --- a/custom/conf/app.example.ini +++ b/custom/conf/app.example.ini @@ -1285,6 +1285,9 @@ LEVEL = Info ;; Number of line of codes shown for a code comment ;CODE_COMMENT_LINES = 4 ;; +;; Maximum number of lines a single (multi-line) code comment may span. 0 means no limit. +;MAX_CODE_COMMENT_LINES = 50 +;; ;; Max size of files to be displayed (default is 8MiB) ;MAX_DISPLAY_FILE_SIZE = 8388608 ;; diff --git a/models/forgejo_migrations/v16b_add_comment_line_count.go b/models/forgejo_migrations/v16b_add_comment_line_count.go new file mode 100644 index 0000000000..4ffe81c58d --- /dev/null +++ b/models/forgejo_migrations/v16b_add_comment_line_count.go @@ -0,0 +1,25 @@ +// Copyright 2026 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package forgejo_migrations + +import "code.forgejo.org/xorm/xorm" + +func init() { + registerMigration(&Migration{ + Description: "add extra_lines_count to comment for multi-line review comments", + Upgrade: addCommentExtraLinesCount, + }) +} + +func addCommentExtraLinesCount(x *xorm.Engine) error { + type Comment struct { + ExtraLinesCount int64 `xorm:"NOT NULL DEFAULT 0"` + } + + _, err := x.SyncWithOptions( + xorm.SyncOptions{IgnoreDropIndices: true}, + new(Comment), + ) + return err +} diff --git a/models/issues/comment.go b/models/issues/comment.go index e0ea1e111d..624e4352e1 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -283,6 +283,7 @@ type Comment struct { CommitID int64 Line int64 // - previous line / + proposed line + ExtraLinesCount int64 `xorm:"NOT NULL DEFAULT 0"` // number of additional lines after Line (0 = single line) TreePath string Content string `xorm:"LONGTEXT"` ContentVersion int `xorm:"NOT NULL DEFAULT 0"` @@ -751,6 +752,58 @@ func (c *Comment) UnsignedLine() uint64 { return uint64(c.Line) } +// DisplayLine returns the signed line number where the comment should be displayed +// in the diff view. For multi-line comments, this is the last line of the range. +func (c *Comment) DisplayLine() int64 { + if c.Line < 0 { + return c.Line - c.ExtraLinesCount + } + return c.Line + c.ExtraLinesCount +} + +// UnsignedDisplayLine returns the unsigned (absolute) display line number. +// For multi-line comments, this is the last line of the range (UnsignedLine + ExtraLinesCount). +func (c *Comment) UnsignedDisplayLine() uint64 { + return c.UnsignedLine() + uint64(c.ExtraLinesCount) +} + +// resolveLineAtHead checks whether a specific line is still present at the given head commit. +// For positive lines (proposed side), uses git blame --reverse. +// For negative lines (previous side), uses diff + FindAdjustedLineNumber. +func (c *Comment) resolveLineAtHead(gitRepo *git.Repository, lineNum uint64, currentHead string) (*git.ReverseLineBlame, error) { + if c.Line > 0 { + blame, err := gitRepo.ReverseLineBlame(c.CommitSHA, c.TreePath, lineNum, currentHead) + if err != nil { + return nil, fmt.Errorf("ReverseLineBlame for line %d: %w", lineNum, err) + } + return blame, nil + } + + // For comments on removed lines, diff the commit the line was known to exist in against the head + // being viewed, then locate the line in that diff. + var buffer bytes.Buffer + if err := git.GetRepoRawDiffForFile(gitRepo, c.CommitSHA, currentHead, git.RawDiffNormal, c.TreePath, &buffer); err != nil { + return nil, fmt.Errorf("failed to get diff: %w", err) + } + diff := buffer.String() + + adjustedLine, err := git.FindAdjustedLineNumber(c.Patch, int64(lineNum), strings.NewReader(diff)) + if err != nil && errors.Is(err, git.ErrLineNotFound) { + return &git.ReverseLineBlame{ + CommitID: "", // not currentHead — indicates the line is outdated + LineNumber: lineNum, + FilePath: c.TreePath, + }, nil + } else if err != nil { + return nil, fmt.Errorf("FindAdjustedLineNumber for line %d: %w", lineNum, err) + } + return &git.ReverseLineBlame{ + CommitID: currentHead, + LineNumber: uint64(adjustedLine.Left), + FilePath: c.TreePath, + }, nil +} + func (c *Comment) ResolveCurrentLine(ctx context.Context, repo *repo_model.Repository, currentHead string) (*git.ReverseLineBlame, error) { if c.reverseLineBlame != nil { return c.reverseLineBlame, nil @@ -774,43 +827,15 @@ func (c *Comment) ResolveCurrentLine(ctx context.Context, repo *repo_model.Repos } defer closer.Close() - var reverseBlame *git.ReverseLineBlame - if c.Line > 0 { - var err error - reverseBlame, err = gitRepo.ReverseLineBlame(c.CommitSHA, c.TreePath, c.UnsignedLine(), currentHead) - if err != nil { - return "", fmt.Errorf("failed to perform `git blame --reverse` to resolve current line for comment (id=%d): %w", c.ID, err) - } - } else { - // For comments on removed lines, perform a `git diff` between the last commit that the line of code was - // known to exist (which is recorded as CommitSHA) and the requested head. Then inspect the diff to verify - // that the removed line of code is present in the diff. - buffer := bytes.Buffer{} - err := git.GetRepoRawDiffForFile(gitRepo, c.CommitSHA, currentHead, git.RawDiffNormal, c.TreePath, &buffer) - if err != nil { - return "", fmt.Errorf("failed to get diff: %w", err) - } - - diff := buffer.String() - adjustedLine, err := git.FindAdjustedLineNumber(c.Patch, int64(c.UnsignedLine()), strings.NewReader(diff)) - if err != nil && errors.Is(err, git.ErrLineNotFound) { - // Line not found in the diff. Don't treat this as an error, because that would break the caching -- - // instead, return a blame where CommitID != headCommitID, which will be an indicator to callers (for - // both resolution methods) that the line of code is outdated in the diff. - reverseBlame = &git.ReverseLineBlame{ - CommitID: "", // not currentHead - LineNumber: c.UnsignedLine(), - FilePath: c.TreePath, - } - } else if err != nil { - return "", fmt.Errorf("failed in finding adjusted line number: %w", err) - } else { - reverseBlame = &git.ReverseLineBlame{ - CommitID: currentHead, - LineNumber: uint64(adjustedLine.Left), - FilePath: c.TreePath, - } - } + // On the previous side the patch is cut around the last line of the range, so resolve that line + // (for single-line comments and the proposed side it equals UnsignedLine). + resolveLine := c.UnsignedLine() + if c.Line < 0 { + resolveLine = c.UnsignedDisplayLine() + } + reverseBlame, err := c.resolveLineAtHead(gitRepo, resolveLine, currentHead) + if err != nil { + return "", err } data, err := json.Marshal(reverseBlame) @@ -834,6 +859,66 @@ func (c *Comment) ResolveCurrentLine(ctx context.Context, repo *repo_model.Repos return c.reverseLineBlame, nil } +// CheckLineRangeValid reports whether a multi-line comment range can still be placed at the given head. +// Previous (negative) side: only the last line can be located in the cut patch, so only it is checked. +// Proposed (positive) side: modified lines are tolerated; only a line landing at an unexpected offset +// (lines inserted/removed inside the range) invalidates it. Uses the same caching pattern as ResolveCurrentLine. +func (c *Comment) CheckLineRangeValid(ctx context.Context, repo *repo_model.Repository, currentHead string) (bool, error) { + if c.ExtraLinesCount <= 0 { + return true, nil + } + + resultJSON, err := cache.GetString(fmt.Sprintf("comment.ResolveRange;ID=%d;HEAD=%s", c.ID, currentHead), func() (string, error) { + gitRepo, closer, err := gitrepo.RepositoryFromContextOrOpen(ctx, repo) + if err != nil { + return "", fmt.Errorf("failed to open repo: %w", err) + } + defer closer.Close() + + // Previous side: only the last line of the range can be located in the cut patch, so validate it. + if c.Line < 0 { + blame, err := c.resolveLineAtHead(gitRepo, c.UnsignedDisplayLine(), currentHead) + if err != nil { + return "", err + } + if blame.CommitID != currentHead { + return "invalid", nil + } + return "valid", nil + } + + // Proposed side: resolve the first line; the others are expected to follow it consecutively. + anchorBlame, err := c.resolveLineAtHead(gitRepo, c.UnsignedLine(), currentHead) + if err != nil { + return "", err + } + if anchorBlame.CommitID != currentHead { + return "invalid", nil + } + anchorResolvedLine := anchorBlame.LineNumber + + // A line that no longer resolves is treated as "modified but present" and tolerated; only a + // resolved line landing at an unexpected offset (lines inserted/removed inside the range) is a break. + startLine := c.UnsignedLine() + for i := int64(1); i <= c.ExtraLinesCount; i++ { + blame, err := c.resolveLineAtHead(gitRepo, startLine+uint64(i), currentHead) + if err != nil { + return "", err + } + if blame.CommitID == currentHead && blame.LineNumber != anchorResolvedLine+uint64(i) { + return "invalid", nil + } + } + + return "valid", nil + }) + if err != nil { + return false, err + } + + return resultJSON == "valid", nil +} + // CodeCommentLink returns the url to a comment in code func (c *Comment) CodeCommentLink(ctx context.Context) string { err := c.LoadIssue(ctx) @@ -914,6 +999,7 @@ func CreateComment(ctx context.Context, opts *CreateCommentOptions) (_ *Comment, CommitID: opts.CommitID, CommitSHA: opts.CommitSHA, Line: opts.LineNum, + ExtraLinesCount: opts.ExtraLinesCount, Content: opts.Content, OldTitle: opts.OldTitle, NewTitle: opts.NewTitle, @@ -1090,6 +1176,7 @@ type CreateCommentOptions struct { CommitSHA string Patch string LineNum int64 + ExtraLinesCount int64 TreePath string ReviewID int64 Content string diff --git a/models/issues/comment_code.go b/models/issues/comment_code.go index cc46541743..45ef55f163 100644 --- a/models/issues/comment_code.go +++ b/models/issues/comment_code.go @@ -46,6 +46,19 @@ func newCodeConversationsAtLineAndTreePath(ctx context.Context, comments []*Comm // when the user views a different commit in the PR, and it will always appear on the "Conversations" tab. continue } + + // For multi-line comments, verify that the full line range is still valid and contiguous at head. + // If lines were inserted/removed/reordered within the range, the comment would be displayed + // at wrong lines — skip it in this view (it remains visible on the "Conversations" tab). + if comment.ExtraLinesCount > 0 && blame != nil { + valid, err := comment.CheckLineRangeValid(ctx, repo, headCommitID) + if err != nil { + log.Warn("CheckLineRangeValid failed for comment %d: %s", comment.ID, err.Error()) + } else if !valid { + continue + } + } + tree.insertComment(comment, blame) } return tree, nil @@ -53,12 +66,17 @@ func newCodeConversationsAtLineAndTreePath(ctx context.Context, comments []*Comm func (tree CodeConversationsAtLineAndTreePath) insertComment(comment *Comment, blame *git.ReverseLineBlame) { treePath := comment.TreePath - line := comment.Line + line := comment.DisplayLine() if blame != nil { treePath = blame.FilePath - line = int64(blame.LineNumber) if comment.Line < 0 { - line *= -1 + // On the previous side, ResolveCurrentLine resolves the last line of the range (the display + // line) directly, so blame.LineNumber already is the signed display line. + line = int64(blame.LineNumber) * -1 + } else { + // On the proposed side, blame resolves the first line; the display line is that line shifted + // down by the number of extra lines. + line = int64(blame.LineNumber) + comment.ExtraLinesCount } } @@ -117,7 +135,8 @@ func fetchCodeCommentsByReview(ctx context.Context, issue *Issue, doer *user_mod if pathToLineToComment[comment.TreePath] == nil { pathToLineToComment[comment.TreePath] = make(map[int64][]*Comment) } - pathToLineToComment[comment.TreePath][comment.Line] = append(pathToLineToComment[comment.TreePath][comment.Line], comment) + displayLine := comment.DisplayLine() + pathToLineToComment[comment.TreePath][displayLine] = append(pathToLineToComment[comment.TreePath][displayLine], comment) } return pathToLineToComment, nil } diff --git a/models/issues/comment_test.go b/models/issues/comment_test.go index ea59d4f215..5c41db3525 100644 --- a/models/issues/comment_test.go +++ b/models/issues/comment_test.go @@ -151,3 +151,59 @@ func Test_UpdateIssueNumComments(t *testing.T) { issue2 = unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2}) assert.Equal(t, 1, issue2.NumComments) } + +func TestDisplayLine(t *testing.T) { + tests := []struct { + name string + line int64 + count int64 + expected int64 + }{ + {"positive single line", 10, 0, 10}, + {"positive multi-line", 7, 2, 9}, + {"positive large range", 1, 49, 50}, + {"negative single line", -10, 0, -10}, + {"negative multi-line", -7, 2, -9}, + {"negative large range", -1, 49, -50}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + c := &issues_model.Comment{Line: tc.line, ExtraLinesCount: tc.count} + assert.Equal(t, tc.expected, c.DisplayLine()) + }) + } +} + +func TestUnsignedDisplayLine(t *testing.T) { + tests := []struct { + name string + line int64 + count int64 + expected uint64 + }{ + {"positive single line", 10, 0, 10}, + {"positive multi-line", 7, 2, 9}, + {"negative single line", -10, 0, 10}, + {"negative multi-line", -7, 2, 9}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + c := &issues_model.Comment{Line: tc.line, ExtraLinesCount: tc.count} + assert.Equal(t, tc.expected, c.UnsignedDisplayLine()) + }) + } +} + +func TestCheckLineRangeValid_SingleLine(t *testing.T) { + // ExtraLinesCount=0 should return true immediately without any git operations + c := &issues_model.Comment{Line: 10, ExtraLinesCount: 0} + valid, err := c.CheckLineRangeValid(t.Context(), nil, "any-commit-id") + require.NoError(t, err) + assert.True(t, valid) + + // Negative line, ExtraLinesCount=0 + c2 := &issues_model.Comment{Line: -5, ExtraLinesCount: 0} + valid2, err := c2.CheckLineRangeValid(t.Context(), nil, "any-commit-id") + require.NoError(t, err) + assert.True(t, valid2) +} diff --git a/modules/git/diff.go b/modules/git/diff.go index c954a933ba..35368411c1 100644 --- a/modules/git/diff.go +++ b/modules/git/diff.go @@ -319,6 +319,14 @@ func FindAdjustedLineNumber(cutDiff string, originalLine int64, fullDiff io.Read rightLine = beginRight inHunk = true } else if inHunk { + // Added ('+') lines exist only on the right side of the diff and have no left-side line + // number. Skip them before testing the target, otherwise a target line that immediately + // follows a removed line would be matched against the inserted line's text and wrongly + // reported as changed. + if len(lineText) > 0 && lineText[0] == '+' { + rightLine++ + continue + } if leftLine == originalLine { if lineText != endOfCutDiff { return LinePlacement{}, fmt.Errorf( @@ -328,8 +336,6 @@ func FindAdjustedLineNumber(cutDiff string, originalLine int64, fullDiff io.Read return LinePlacement{Left: leftLine, Right: rightLine}, nil } switch lineText[0] { - case '+': - rightLine++ case '-': leftLine++ case '\\': diff --git a/modules/git/diff_test.go b/modules/git/diff_test.go index e4b7ce6ace..36cfd7fe25 100644 --- a/modules/git/diff_test.go +++ b/modules/git/diff_test.go @@ -154,6 +154,34 @@ func TestCutDiffAroundLine(t *testing.T) { assert.Equal(t, expected, minusDiff) } +func TestCutDiffAroundLineMultiRange(t *testing.T) { + // Simulate multi-line comment: lines 2-4 on new side ( + // displayLine=4 -> last line of the comment + // context=3+2=5 -> 3 lines of context + 2 lines of the comment that are above the display line + // ) + // This mirrors how services/pull/review.go computes the patch for multi-line comments: + // displayLine = UnsignedDisplayLine() (last line of range) + // contextLines = setting.UI.CodeCommentLines + extraLinesCount + result, err := CutDiffAroundLine(strings.NewReader(breakingDiff), 4, false, 5) + require.NoError(t, err) + + // Should include lines 1-4 of the new side (--some comment, --some comment 2, -- some comment 3, create or replace...) + assert.Contains(t, result, "--some comment 2") + assert.Contains(t, result, "-- some comment 3") + assert.Contains(t, result, "create or replace procedure") + + // Simulate single line (extraLinesCount=0): displayLine=2, context=3 + singleResult, err := CutDiffAroundLine(strings.NewReader(breakingDiff), 2, false, 3) + require.NoError(t, err) + assert.Contains(t, singleResult, "--some comment 2") + + // Multi-line on exampleDiff: lines 3-5 new side (displayLine=5, context=3+2=5) + multiResult, err := CutDiffAroundLine(strings.NewReader(exampleDiff), 5, false, 5) + require.NoError(t, err) + assert.Contains(t, multiResult, "Build Status") + assert.Contains(t, multiResult, "Docker Pulls") +} + func BenchmarkCutDiffAroundLine(b *testing.B) { for n := 0; n < b.N; n++ { CutDiffAroundLine(strings.NewReader(exampleDiff), 3, true, 3) @@ -291,6 +319,35 @@ index 2d203fb..d0cb63f 100644 assert.Equal(t, LinePlacement{Left: 50, Right: 52}, lineNumber) }) + t.Run("target line is an unchanged line right after a modification", func(t *testing.T) { + // The commented line (49) is a context line immediately following a modified line (48). + // The added ('+') line for the modification must be skipped while locating the old-side line, + // otherwise it would be matched against line 49 and wrongly reported as changed. + cutDiff := `diff --git a/file1.md b/file1.md +--- a/file1.md ++++ b/file1.md +@@ -46,4 +46,4 @@ Line 45 + Line 46 + Line 47 +-Line 48 ++Line 48--modified + Line 49` + diff := `diff --git a/file1.md b/file1.md +index 2d203fb..b21df3f 100644 +--- a/file1.md ++++ b/file1.md +@@ -46,4 +46,4 @@ Line 45 + Line 46 + Line 47 +-Line 48 ++Line 48--modified + Line 49 + Line 50` + lineNumber, err := FindAdjustedLineNumber(cutDiff, 49, strings.NewReader(diff)) + require.NoError(t, err) + assert.Equal(t, LinePlacement{Left: 49, Right: 49}, lineNumber) + }) + t.Run("changes above in the same hunk", func(t *testing.T) { diff := `diff --git a/file1.md b/file1.md index 2d203fb..f35a466 100644 diff --git a/modules/setting/ui.go b/modules/setting/ui.go index abe902dfc6..d86e8e74c7 100644 --- a/modules/setting/ui.go +++ b/modules/setting/ui.go @@ -21,6 +21,7 @@ var UI = struct { PackagesPagingNum int GraphMaxCommitNum int CodeCommentLines int + MaxCodeCommentLines int ReactionMaxUserNum int MaxDisplayFileSize int64 ShowUserEmail bool @@ -81,6 +82,7 @@ var UI = struct { PackagesPagingNum: 20, GraphMaxCommitNum: 100, CodeCommentLines: 4, + MaxCodeCommentLines: 50, ReactionMaxUserNum: 10, MaxDisplayFileSize: 8388608, DefaultTheme: `forgejo-auto`, diff --git a/modules/structs/pull_review.go b/modules/structs/pull_review.go index f89c1f2a63..cb6cf6bbe0 100644 --- a/modules/structs/pull_review.go +++ b/modules/structs/pull_review.go @@ -65,6 +65,8 @@ type PullReviewComment struct { DiffHunk string `json:"diff_hunk"` LineNum uint64 `json:"position"` OldLineNum uint64 `json:"original_position"` + // number of additional lines after the commented line (0 = single line comment) + ExtraLinesCount int64 `json:"extra_lines_count"` HTMLURL string `json:"html_url"` HTMLPullURL string `json:"pull_request_url"` @@ -87,6 +89,8 @@ type CreatePullReviewComment struct { OldLineNum int64 `json:"old_position"` // if comment to new file line or 0 NewLineNum int64 `json:"new_position"` + // number of additional lines after the commented line (0 = single line comment) + ExtraLinesCount int64 `json:"extra_lines_count"` } type CreatePullReviewCommentOptions CreatePullReviewComment diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index e28c1edafc..8180eceee6 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -331,6 +331,11 @@ func CreatePullReviewComment(ctx *context.APIContext) { line = opts.OldLineNum * -1 } + if err := pull_service.ValidateCodeCommentLineRange(opts.ExtraLinesCount); err != nil { + ctx.Error(http.StatusUnprocessableEntity, "invalid extra_lines_count", err) + return + } + comment, err := pull_service.CreateCodeCommentKnownReviewID(ctx, ctx.Doer, pr.Issue.Repo, @@ -340,6 +345,7 @@ func CreatePullReviewComment(ctx *context.APIContext) { pr.MergeBase, review.CommitID, line, + opts.ExtraLinesCount, review.ID, nil, ) @@ -496,6 +502,11 @@ func CreatePullReview(ctx *context.APIContext) { // create review comments for _, c := range opts.Comments { + if err := pull_service.ValidateCodeCommentLineRange(c.ExtraLinesCount); err != nil { + ctx.Error(http.StatusUnprocessableEntity, "invalid extra_lines_count", err) + return + } + line := c.NewLineNum if c.OldLineNum > 0 { line = c.OldLineNum * -1 @@ -506,6 +517,7 @@ func CreatePullReview(ctx *context.APIContext) { ctx.Repo.GitRepo, pr.Issue, line, + c.ExtraLinesCount, c.Body, c.Path, true, // pending review diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go index 401f100d84..e5e0697aa9 100644 --- a/routers/web/repo/pull_review.go +++ b/routers/web/repo/pull_review.go @@ -89,6 +89,11 @@ func CreateCodeComment(ctx *context.Context) { signedLine *= -1 } + if err := pull_service.ValidateCodeCommentLineRange(form.ExtraLinesCount); err != nil { + ctx.Error(http.StatusBadRequest, err.Error()) + return + } + var attachments []string if setting.Attachment.Enabled { attachments = form.Files @@ -117,6 +122,7 @@ func CreateCodeComment(ctx *context.Context) { ctx.Repo.GitRepo, issue, signedLine, + form.ExtraLinesCount, form.Content, form.TreePath, pendingReview, diff --git a/routers/web/repo/pull_review_test.go b/routers/web/repo/pull_review_test.go index c068f575dd..e69b91f8f8 100644 --- a/routers/web/repo/pull_review_test.go +++ b/routers/web/repo/pull_review_test.go @@ -6,15 +6,20 @@ package repo import ( "net/http" "net/http/httptest" + "strconv" "testing" "forgejo.org/models/db" issues_model "forgejo.org/models/issues" "forgejo.org/models/unittest" "forgejo.org/modules/git" + "forgejo.org/modules/setting" "forgejo.org/modules/templates" + "forgejo.org/modules/test" + "forgejo.org/modules/web" "forgejo.org/services/context" "forgejo.org/services/contexttest" + "forgejo.org/services/forms" "forgejo.org/services/pull" "github.com/stretchr/testify/assert" @@ -51,7 +56,7 @@ func TestRenderConversation(t *testing.T) { var preparedComment *issues_model.Comment run("prepare", func(t *testing.T, ctx *context.Context, resp *httptest.ResponseRecorder) { comment, err := pull.CreateCodeComment(ctx, pr.Issue.Poster, ctx.Repo.GitRepo, pr.Issue, - 1, "content", "", false, 0, pr.MergeBase, + 1, 0, "content", "", false, 0, pr.MergeBase, prHeadCommitID, nil) require.NoError(t, err) @@ -111,4 +116,92 @@ func TestRenderConversation(t *testing.T) { assert.Equal(t, http.StatusOK, resp.Code) assert.NotContains(t, resp.Body.String(), `status-page-500`) }) + + // Test multi-line comment rendering + var multiLineComment *issues_model.Comment + run("prepare multi-line comment", func(t *testing.T, ctx *context.Context, resp *httptest.ResponseRecorder) { + comment, err := pull.CreateCodeComment(ctx, pr.Issue.Poster, ctx.Repo.GitRepo, pr.Issue, + 1, 2, "multi-line content", "", false, 0, pr.MergeBase, + prHeadCommitID, nil) + require.NoError(t, err) + assert.EqualValues(t, 2, comment.ExtraLinesCount) + multiLineComment = comment + }) + if !assert.NotNil(t, multiLineComment) { + return + } + run("timeline multi-line comment renders", func(t *testing.T, ctx *context.Context, resp *httptest.ResponseRecorder) { + renderConversation(ctx, multiLineComment, "timeline") + body := resp.Body.String() + assert.Contains(t, body, `
50 + }) + + CreateCodeComment(ctx) + + assert.Equal(t, http.StatusBadRequest, resp.Code) } diff --git a/services/convert/pull_review.go b/services/convert/pull_review.go index 97be118a83..1444024fa8 100644 --- a/services/convert/pull_review.go +++ b/services/convert/pull_review.go @@ -81,19 +81,20 @@ func ToPullReviewList(ctx context.Context, rl []*issues_model.Review, doer *user // ToPullReviewCommentList convert the CodeComments of an review to it's api format func ToPullReviewComment(ctx context.Context, review *issues_model.Review, comment *issues_model.Comment, doer *user_model.User) (*api.PullReviewComment, error) { apiComment := &api.PullReviewComment{ - ID: comment.ID, - Body: comment.Content, - Poster: ToUser(ctx, comment.Poster, doer), - Resolver: ToUser(ctx, comment.ResolveDoer, doer), - ReviewID: review.ID, - Created: comment.CreatedUnix.AsTime(), - Updated: comment.UpdatedUnix.AsTime(), - Path: comment.TreePath, - CommitID: comment.CommitSHA, - OrigCommitID: comment.OldRef, - DiffHunk: patch2diff(comment.Patch), - HTMLURL: comment.HTMLURL(ctx), - HTMLPullURL: review.Issue.HTMLURL(), + ID: comment.ID, + Body: comment.Content, + Poster: ToUser(ctx, comment.Poster, doer), + Resolver: ToUser(ctx, comment.ResolveDoer, doer), + ReviewID: review.ID, + Created: comment.CreatedUnix.AsTime(), + Updated: comment.UpdatedUnix.AsTime(), + Path: comment.TreePath, + CommitID: comment.CommitSHA, + OrigCommitID: comment.OldRef, + DiffHunk: patch2diff(comment.Patch), + ExtraLinesCount: comment.ExtraLinesCount, + HTMLURL: comment.HTMLURL(ctx), + HTMLPullURL: review.Issue.HTMLURL(), } if comment.Line < 0 { diff --git a/services/forms/repo_form.go b/services/forms/repo_form.go index d1a5b521f8..4d2c592eb1 100644 --- a/services/forms/repo_form.go +++ b/services/forms/repo_form.go @@ -465,16 +465,17 @@ func (f *MergePullRequestForm) Validate(req *http.Request, errs binding.Errors) // CodeCommentForm form for adding code comments for PRs type CodeCommentForm struct { - Origin string `binding:"Required;In(timeline,diff)"` - Content string `binding:"Required"` - Side string `binding:"Required;In(previous,proposed)"` - Line int64 - TreePath string `form:"path" binding:"Required"` - SingleReview bool `form:"single_review"` - Reply int64 `form:"reply"` - BeforeCommitID string - LatestCommitID string - Files []string + Origin string `binding:"Required;In(timeline,diff)"` + Content string `binding:"Required"` + Side string `binding:"Required;In(previous,proposed)"` + Line int64 + ExtraLinesCount int64 `form:"extra_lines_count"` + TreePath string `form:"path" binding:"Required"` + SingleReview bool `form:"single_review"` + Reply int64 `form:"reply"` + BeforeCommitID string + LatestCommitID string + Files []string } // Validate validates the fields diff --git a/services/mailer/incoming/incoming_handler.go b/services/mailer/incoming/incoming_handler.go index 6fdf00ca59..7d6b2c6361 100644 --- a/services/mailer/incoming/incoming_handler.go +++ b/services/mailer/incoming/incoming_handler.go @@ -148,6 +148,7 @@ func (h *ReplyHandler) Handle(ctx context.Context, content *MailContent, doer *u nil, issue, comment.Line, + 0, // extraLinesCount: a reply to a comment inherit single line content.Content, comment.TreePath, false, // not pending review but a single review diff --git a/services/pull/review.go b/services/pull/review.go index d8e04675ea..a69c0edc92 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -46,13 +46,38 @@ func checkInvalidation(ctx context.Context, c *issues_model.Comment, repo *repo_ reverseBlame, err := c.ResolveCurrentLine(ctx, repo, newCommitID) if err != nil { log.Warn("ResolveCurrentLine failed: %s", err.Error()) - } else if reverseBlame.CommitID != newCommitID { + return nil + } + if reverseBlame.CommitID != newCommitID { c.Invalidated = true return issues_model.UpdateCommentInvalidate(ctx, c) } + + // For multi-line comments, check additional lines in the range + if c.ExtraLinesCount > 0 { + invalidated, err := checkMultiLineInvalidation(ctx, c, repo, newCommitID) + if err != nil { + log.Warn("checkMultiLineInvalidation failed: %s", err.Error()) + } else if invalidated { + c.Invalidated = true + return issues_model.UpdateCommentInvalidate(ctx, c) + } + } + return nil } +// checkMultiLineInvalidation checks if any additional line in a multi-line comment range +// has been changed. Returns true if the comment should be invalidated. +// Uses cached results via Comment.CheckLineRangeValid. +func checkMultiLineInvalidation(ctx context.Context, c *issues_model.Comment, repo *repo_model.Repository, newCommitID string) (bool, error) { + valid, err := c.CheckLineRangeValid(ctx, repo, newCommitID) + if err != nil { + return false, err + } + return !valid, nil +} + // InvalidateCodeComments will lookup the prs for code comments which got invalidated by change func InvalidateCodeComments(ctx context.Context, prs issues_model.PullRequestList, doer *user_model.User, repo *repo_model.Repository, newCommitID string) error { if len(prs) == 0 { @@ -77,9 +102,22 @@ func InvalidateCodeComments(ctx context.Context, prs issues_model.PullRequestLis return nil } +// ValidateCodeCommentLineRange validates the extra_lines_count of a (multi-line) code comment: it must +// not be negative and the resulting range must not span more than setting.UI.MaxCodeCommentLines lines +// (0 = no limit). It is the single source of truth shared by the web and API creation handlers. +func ValidateCodeCommentLineRange(extraLinesCount int64) error { + if extraLinesCount < 0 { + return fmt.Errorf("extra_lines_count must be >= 0") + } + if setting.UI.MaxCodeCommentLines > 0 && extraLinesCount+1 > int64(setting.UI.MaxCodeCommentLines) { + return fmt.Errorf("a code comment may span at most %d lines", setting.UI.MaxCodeCommentLines) + } + return nil +} + // CreateCodeComment creates a comment on the code line func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git.Repository, - issue *issues_model.Issue, line int64, content, treePath string, pendingReview bool, + issue *issues_model.Issue, line, extraLinesCount int64, content, treePath string, pendingReview bool, replyReviewID int64, beforeCommitID, latestCommitID string, attachments []string, ) (*issues_model.Comment, error) { var ( @@ -115,6 +153,7 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. beforeCommitID, latestCommitID, line, + extraLinesCount, replyReviewID, attachments, ) @@ -158,6 +197,7 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. beforeCommitID, latestCommitID, line, + extraLinesCount, review.ID, attachments, ) @@ -180,7 +220,7 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. // CreateCodeCommentKnownReviewID creates a plain code comment at the specified line / path func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, issue *issues_model.Issue, content, treePath, beforeCommitID, afterCommitID string, - line, reviewID int64, attachments []string, + line, extraLinesCount, reviewID int64, attachments []string, ) (*issues_model.Comment, error) { var commitID, blamedCommitID, patch string blamedLine := line @@ -248,12 +288,23 @@ func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User, blame, err := gitRepo.ReverseLineBlame(beforeCommitID, treePath, uint64(-1*line), afterCommitID) if err != nil { return nil, fmt.Errorf("ReverseLineBlame[%s, %s, %d, %s]: %w", beforeCommitID, treePath, -1*line, afterCommitID, err) - } else if blame.CommitID == afterCommitID { - // Although this is a comment on the "previous" side of the diff, the reverse blame indicates that the line - // of code still exists in the commit being viewed (eg. it was a comment on a white line in the left-side of - // the diff, not a red removed line). In order to record the right information for where to place this - // commit, we'll convert this into a right-hand comment -- using the present line number that the reverse - // blame gave us: + } + + // Convert to a right-hand (proposed) comment only when EVERY line of the range still exists at head + // (whole selection unchanged); if any line was removed or modified, keep it on the previous side. + rangeStillExists := blame.CommitID == afterCommitID + for i := int64(1); rangeStillExists && i <= extraLinesCount; i++ { + lineBlame, err := gitRepo.ReverseLineBlame(beforeCommitID, treePath, uint64(-1*line)+uint64(i), afterCommitID) + if err != nil { + return nil, fmt.Errorf("ReverseLineBlame[%s, %s, %d, %s]: %w", beforeCommitID, treePath, -1*line+i, afterCommitID, err) + } + if lineBlame.CommitID != afterCommitID { + rangeStillExists = false + } + } + + switch { + case rangeStillExists: commit, lineres, err := gitRepo.LineBlame(afterCommitID, treePath, blame.LineNumber) if err == nil { blamedCommitID = commit.ID.String() @@ -261,7 +312,12 @@ func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User, } else if !errors.Is(err, git.ErrBlameFileDoesNotExist) && !errors.Is(err, git.ErrBlameFileNotEnoughLines) { return nil, fmt.Errorf("LineBlame[%s, %s, %s, %d]: %w", pr.GetGitRefName(), gitRepo.Path, treePath, line, err) } - } else { + case blame.CommitID == afterCommitID: + // First line still exists but a later one changed + blamedCommitID = beforeCommitID + // retain negative line numbering to identify we're commenting on the "previous" side of the diff + blamedLine = line + default: blamedCommitID = blame.CommitID // retain negative line numbering to identify we're commenting on the "previous" side of the diff blamedLine = -1 * int64(blame.LineNumber) @@ -292,25 +348,29 @@ func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User, _ = writer.Close() }() - patch, err = git.CutDiffAroundLine(reader, int64((&issues_model.Comment{Line: line}).UnsignedLine()), line < 0, setting.UI.CodeCommentLines) + // For multi-line comments, center the patch on the last line and expand context to include the full range + displayLine := int64((&issues_model.Comment{Line: line, ExtraLinesCount: extraLinesCount}).UnsignedDisplayLine()) + contextLines := setting.UI.CodeCommentLines + int(extraLinesCount) + patch, err = git.CutDiffAroundLine(reader, displayLine, line < 0, contextLines) if err != nil { log.Error("Error whilst generating patch: %v", err) return nil, err } } return issues_model.CreateComment(ctx, &issues_model.CreateCommentOptions{ - Type: issues_model.CommentTypeCode, - Doer: doer, - Repo: repo, - Issue: issue, - Content: content, - LineNum: blamedLine, - TreePath: treePath, - CommitSHA: blamedCommitID, - ReviewID: reviewID, - Patch: patch, - Invalidated: invalidated, - Attachments: attachments, + Type: issues_model.CommentTypeCode, + Doer: doer, + Repo: repo, + Issue: issue, + Content: content, + LineNum: blamedLine, + ExtraLinesCount: extraLinesCount, + TreePath: treePath, + CommitSHA: blamedCommitID, + ReviewID: reviewID, + Patch: patch, + Invalidated: invalidated, + Attachments: attachments, }) } diff --git a/templates/repo/diff/comment_form.tmpl b/templates/repo/diff/comment_form.tmpl index 7b64be71a0..6d8dc40f3a 100644 --- a/templates/repo/diff/comment_form.tmpl +++ b/templates/repo/diff/comment_form.tmpl @@ -5,6 +5,7 @@ + diff --git a/templates/repo/diff/comments.tmpl b/templates/repo/diff/comments.tmpl index 3128149c75..ec1f8b1327 100644 --- a/templates/repo/diff/comments.tmpl +++ b/templates/repo/diff/comments.tmpl @@ -31,6 +31,9 @@ {{end}}
+ {{if gt .ExtraLinesCount 0}} + Lines {{.UnsignedLine}}-{{.UnsignedDisplayLine}} + {{end}} {{if .Invalidated}} {{$referenceUrl := printf "%s#%s" $.root.Issue.Link .HashTag}} diff --git a/templates/repo/diff/conversation.tmpl b/templates/repo/diff/conversation.tmpl index 00e4afdfb1..e8fd3917c4 100644 --- a/templates/repo/diff/conversation.tmpl +++ b/templates/repo/diff/conversation.tmpl @@ -3,7 +3,7 @@ {{$resolveDoer := (index .comments 0).ResolveDoer}} {{$isNotPending := (not (eq (index .comments 0).Review.Type 0))}} {{$referenceUrl := printf "%s#%s" $.Issue.Link (index .comments 0).HashTag}} -
+
{{if $resolved}}
diff --git a/templates/repo/issue/view_content/conversation.tmpl b/templates/repo/issue/view_content/conversation.tmpl index 436b6d9b12..de76745326 100644 --- a/templates/repo/issue/view_content/conversation.tmpl +++ b/templates/repo/issue/view_content/conversation.tmpl @@ -12,7 +12,10 @@ {{end}}
-
+
+ {{if gt (index .comments 0).ExtraLinesCount 0}} + Lines {{(index .comments 0).UnsignedLine}}-{{(index .comments 0).UnsignedDisplayLine}} + {{end}} {{if or $invalid $resolved}}