Skip to content

Allow code review comments crossing commit with right line number #35077

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 38 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
345ae04
Allow code review comments display cross commits even if the head bra…
lunny Jul 13, 2025
f8cea67
improvements
lunny Jul 14, 2025
eddd21b
Refactor calc mergebase
lunny Jul 14, 2025
5ba2216
improvements
lunny Jul 14, 2025
aaa5364
Add migrations for wrong negative line number in the review code comment
lunny Jul 14, 2025
2256fa7
Fix bug
lunny Jul 14, 2025
65a01c2
Fix bug
lunny Jul 15, 2025
968b2c5
Fix bug
lunny Jul 15, 2025
e895298
Fix bug
lunny Jul 15, 2025
c68718d
Fix test
lunny Jul 15, 2025
01476db
Fix test
lunny Jul 15, 2025
c58cc46
Fix bug
lunny Jul 15, 2025
cc62a93
Remove unused router and test
lunny Jul 16, 2025
c4f7e31
Fix bug
lunny Jul 16, 2025
64dec56
Fix bug
lunny Jul 16, 2025
e279f0e
Rename
lunny Jul 16, 2025
aaed5c9
remove the code extract to other pull requests
lunny Jul 16, 2025
45931b3
revert unnecessary change
lunny Jul 19, 2025
1ecac1a
Don't use migrations but use a doctor to fix old negative line number…
lunny Jul 19, 2025
99b5634
Fix
lunny Jul 19, 2025
be18643
Add test and comment
lunny Jul 20, 2025
68e5b28
add test
lunny Jul 20, 2025
cbd73a5
add test
lunny Jul 20, 2025
1556911
Fix test
lunny Jul 22, 2025
8b4e077
Fix bug
lunny Jul 23, 2025
0806da7
add missing file
lunny Jul 23, 2025
5192300
Merge branch 'main' into lunny/code_review_line_number
lunny Jul 23, 2025
5bb19fd
Remove unnecessary code
lunny Jul 23, 2025
664da3a
Remove unnecessary code
lunny Jul 23, 2025
105179a
Fix lint
lunny Jul 23, 2025
1fb7b01
improvements
lunny Jul 23, 2025
8fbc1e0
Fix test
lunny Jul 23, 2025
507e974
Merge branch 'main' into lunny/code_review_line_number
lunny Jul 23, 2025
648c479
Fix bug
lunny Jul 23, 2025
73b1d11
Fix bug
lunny Jul 23, 2025
c9ce1cc
Fix bug
lunny Jul 23, 2025
6f6d956
Fix test
lunny Jul 24, 2025
ff7a661
Merge branch 'main' into lunny/code_review_line_number
lunny Jul 24, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions models/issues/comment.go
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,8 @@ type Comment struct {
UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"`

// Reference issue in commit message
CommitSHA string `xorm:"VARCHAR(64)"`
BeforeCommitID string `xorm:"VARCHAR(64)"`
CommitSHA string `xorm:"VARCHAR(64)"`

Attachments []*repo_model.Attachment `xorm:"-"`
Reactions ReactionList `xorm:"-"`
Expand Down Expand Up @@ -764,6 +765,10 @@ func (c *Comment) CodeCommentLink(ctx context.Context) string {
return fmt.Sprintf("%s/files#%s", c.Issue.Link(), c.HashTag())
}

func GetCodeCommentRefName(prIndex, commentID int64, suffix string) string {
return fmt.Sprintf("refs/pull/%d/code-comment-%d", prIndex, commentID)
}

// CreateComment creates comment with context
func CreateComment(ctx context.Context, opts *CreateCommentOptions) (_ *Comment, err error) {
return db.WithTx2(ctx, func(ctx context.Context) (*Comment, error) {
Expand Down Expand Up @@ -796,6 +801,7 @@ func CreateComment(ctx context.Context, opts *CreateCommentOptions) (_ *Comment,
AssigneeID: opts.AssigneeID,
AssigneeTeamID: opts.AssigneeTeamID,
CommitID: opts.CommitID,
BeforeCommitID: opts.BeforeCommitID,
CommitSHA: opts.CommitSHA,
Line: opts.LineNum,
Content: opts.Content,
Expand Down Expand Up @@ -965,7 +971,8 @@ type CreateCommentOptions struct {
OldRef string
NewRef string
CommitID int64
CommitSHA string
BeforeCommitID string // before commit id when creating this code comment
CommitSHA string // after commit id when creating this code comment, ref commit id for other comment
Patch string
LineNum int64
TreePath string
Expand Down
51 changes: 21 additions & 30 deletions models/issues/comment_code.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,46 +9,52 @@ import (

"code.gitea.io/gitea/models/db"
"code.gitea.io/gitea/models/renderhelper"
repo_model "code.gitea.io/gitea/models/repo"
user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/markup/markdown"

"xorm.io/builder"
)

// CodeComments represents comments on code by using this structure: FILENAME -> LINE (+ == proposed; - == previous) -> COMMENTS
type CodeComments map[string]map[int64][]*Comment
type CodeComments map[string][]*Comment

func (cc CodeComments) AllComments() []*Comment {
var allComments []*Comment
for _, comments := range cc {
allComments = append(allComments, comments...)
}
return allComments
}

// FetchCodeComments will return a 2d-map: ["Path"]["Line"] = Comments at line
func FetchCodeComments(ctx context.Context, issue *Issue, currentUser *user_model.User, showOutdatedComments bool) (CodeComments, error) {
return fetchCodeCommentsByReview(ctx, issue, currentUser, nil, showOutdatedComments)
func FetchCodeComments(ctx context.Context, repo *repo_model.Repository, issueID int64, currentUser *user_model.User, showOutdatedComments bool) (CodeComments, error) {
return fetchCodeCommentsByReview(ctx, repo, issueID, currentUser, nil, showOutdatedComments)
}

func fetchCodeCommentsByReview(ctx context.Context, issue *Issue, currentUser *user_model.User, review *Review, showOutdatedComments bool) (CodeComments, error) {
pathToLineToComment := make(CodeComments)
func fetchCodeCommentsByReview(ctx context.Context, repo *repo_model.Repository, issueID int64, currentUser *user_model.User, review *Review, showOutdatedComments bool) (CodeComments, error) {
codeCommentsPathMap := make(CodeComments)
if review == nil {
review = &Review{ID: 0}
}
opts := FindCommentsOptions{
Type: CommentTypeCode,
IssueID: issue.ID,
IssueID: issueID,
ReviewID: review.ID,
}

comments, err := findCodeComments(ctx, opts, issue, currentUser, review, showOutdatedComments)
comments, err := FindCodeComments(ctx, opts, repo, currentUser, review, showOutdatedComments)
if err != nil {
return nil, err
}

for _, comment := range comments {
if pathToLineToComment[comment.TreePath] == nil {
pathToLineToComment[comment.TreePath] = make(map[int64][]*Comment)
}
pathToLineToComment[comment.TreePath][comment.Line] = append(pathToLineToComment[comment.TreePath][comment.Line], comment)
codeCommentsPathMap[comment.TreePath] = append(codeCommentsPathMap[comment.TreePath], comment)
}
return pathToLineToComment, nil
return codeCommentsPathMap, nil
}

func findCodeComments(ctx context.Context, opts FindCommentsOptions, issue *Issue, currentUser *user_model.User, review *Review, showOutdatedComments bool) ([]*Comment, error) {
func FindCodeComments(ctx context.Context, opts FindCommentsOptions, repo *repo_model.Repository, currentUser *user_model.User, review *Review, showOutdatedComments bool) ([]*Comment, error) {
var comments CommentList
if review == nil {
review = &Review{ID: 0}
Expand All @@ -67,10 +73,6 @@ func findCodeComments(ctx context.Context, opts FindCommentsOptions, issue *Issu
return nil, err
}

if err := issue.LoadRepo(ctx); err != nil {
return nil, err
}

if err := comments.LoadPosters(ctx); err != nil {
return nil, err
}
Expand Down Expand Up @@ -110,12 +112,12 @@ func findCodeComments(ctx context.Context, opts FindCommentsOptions, issue *Issu
return nil, err
}

if err := comment.LoadReactions(ctx, issue.Repo); err != nil {
if err := comment.LoadReactions(ctx, repo); err != nil {
return nil, err
}

var err error
rctx := renderhelper.NewRenderContextRepoComment(ctx, issue.Repo, renderhelper.RepoCommentOptions{
rctx := renderhelper.NewRenderContextRepoComment(ctx, repo, renderhelper.RepoCommentOptions{
FootnoteContextID: strconv.FormatInt(comment.ID, 10),
})
if comment.RenderedContent, err = markdown.RenderString(rctx, comment.Content); err != nil {
Expand All @@ -124,14 +126,3 @@ func findCodeComments(ctx context.Context, opts FindCommentsOptions, issue *Issu
}
return comments[:n], nil
}

// FetchCodeCommentsByLine fetches the code comments for a given treePath and line number
func FetchCodeCommentsByLine(ctx context.Context, issue *Issue, currentUser *user_model.User, treePath string, line int64, showOutdatedComments bool) (CommentList, error) {
opts := FindCommentsOptions{
Type: CommentTypeCode,
IssueID: issue.ID,
TreePath: treePath,
Line: line,
}
return findCodeComments(ctx, opts, issue, currentUser, nil, showOutdatedComments)
}
15 changes: 10 additions & 5 deletions models/issues/comment_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,15 +68,20 @@ func TestFetchCodeComments(t *testing.T) {

issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2})
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
res, err := issues_model.FetchCodeComments(db.DefaultContext, issue, user, false)
res, err := issues_model.FetchCodeComments(db.DefaultContext, issue.Repo, issue.ID, user, false)
assert.NoError(t, err)
assert.Contains(t, res, "README.md")
assert.Contains(t, res["README.md"], int64(4))
assert.Len(t, res["README.md"][4], 1)
assert.Equal(t, int64(4), res["README.md"][4][0].ID)
fourthLineComments := []*issues_model.Comment{}
for _, comment := range res["README.md"] {
if comment.Line == 4 {
fourthLineComments = append(fourthLineComments, comment)
}
}
assert.Len(t, fourthLineComments, 1)
assert.Equal(t, int64(4), fourthLineComments[0].ID)

user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
res, err = issues_model.FetchCodeComments(db.DefaultContext, issue, user2, false)
res, err = issues_model.FetchCodeComments(db.DefaultContext, issue.Repo, issue.ID, user2, false)
assert.NoError(t, err)
assert.Len(t, res, 1)
}
Expand Down
4 changes: 2 additions & 2 deletions models/issues/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ func (issue *Issue) LoadPullRequest(ctx context.Context) (err error) {
return nil
}

func (issue *Issue) loadComments(ctx context.Context) (err error) {
func (issue *Issue) LoadComments(ctx context.Context) (err error) {
return issue.loadCommentsByType(ctx, CommentTypeUndefined)
}

Expand Down Expand Up @@ -344,7 +344,7 @@ func (issue *Issue) LoadAttributes(ctx context.Context) (err error) {
return err
}

if err = issue.loadComments(ctx); err != nil {
if err = issue.LoadComments(ctx); err != nil {
return err
}

Expand Down
28 changes: 6 additions & 22 deletions models/issues/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import (
"code.gitea.io/gitea/models/db"
git_model "code.gitea.io/gitea/models/git"
org_model "code.gitea.io/gitea/models/organization"
pull_model "code.gitea.io/gitea/models/pull"
repo_model "code.gitea.io/gitea/models/repo"
user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/git"
Expand Down Expand Up @@ -156,26 +155,6 @@ func init() {
db.RegisterModel(new(PullRequest))
}

// DeletePullsByBaseRepoID deletes all pull requests by the base repository ID
func DeletePullsByBaseRepoID(ctx context.Context, repoID int64) error {
deleteCond := builder.Select("id").From("pull_request").Where(builder.Eq{"pull_request.base_repo_id": repoID})

// Delete scheduled auto merges
if _, err := db.GetEngine(ctx).In("pull_id", deleteCond).
Delete(&pull_model.AutoMerge{}); err != nil {
return err
}

// Delete review states
if _, err := db.GetEngine(ctx).In("pull_id", deleteCond).
Delete(&pull_model.ReviewState{}); err != nil {
return err
}

_, err := db.DeleteByBean(ctx, &PullRequest{BaseRepoID: repoID})
return err
}

func (pr *PullRequest) String() string {
if pr == nil {
return "<PullRequest nil>"
Expand Down Expand Up @@ -406,11 +385,16 @@ func (pr *PullRequest) getReviewedByLines(ctx context.Context, writer io.Writer)
return nil
}

// GetGitHeadRefName returns git ref for hidden pull request branch
// GetGitHeadRefName returns git head commit id ref for the pull request's branch
func (pr *PullRequest) GetGitHeadRefName() string {
return fmt.Sprintf("%s%d/head", git.PullPrefix, pr.Index)
}

// GetGitMergeRefName returns git merged commit id ref for the pull request
func (pr *PullRequest) GetGitMergeRefName() string {
return fmt.Sprintf("%s%d/merge", git.PullPrefix, pr.Index)
}

func (pr *PullRequest) GetGitHeadBranchRefName() string {
return fmt.Sprintf("%s%s", git.BranchPrefix, pr.HeadBranch)
}
Expand Down
6 changes: 5 additions & 1 deletion models/issues/review.go
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ func (r *Review) LoadCodeComments(ctx context.Context) (err error) {
if err = r.LoadIssue(ctx); err != nil {
return err
}
r.CodeComments, err = fetchCodeCommentsByReview(ctx, r.Issue, nil, r, false)
r.CodeComments, err = fetchCodeCommentsByReview(ctx, r.Issue.Repo, r.Issue.ID, nil, r, false)
return err
}

Expand Down Expand Up @@ -429,6 +429,10 @@ func SubmitReview(ctx context.Context, doer *user_model.User, issue *Issue, revi
defer committer.Close()
sess := db.GetEngine(ctx)

if err := issue.LoadRepo(ctx); err != nil {
return nil, nil, fmt.Errorf("LoadRepo: %w", err)
}

official := false

review, err := GetCurrentReview(ctx, doer, issue)
Expand Down
2 changes: 1 addition & 1 deletion models/issues/review_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func TestReview_LoadCodeComments(t *testing.T) {
assert.NoError(t, review.LoadAttributes(db.DefaultContext))
assert.NoError(t, review.LoadCodeComments(db.DefaultContext))
assert.Len(t, review.CodeComments, 1)
assert.Equal(t, int64(4), review.CodeComments["README.md"][int64(4)][0].Line)
assert.Equal(t, int64(4), review.CodeComments["README.md"][0].Line)
}

func TestReviewType_Icon(t *testing.T) {
Expand Down
1 change: 1 addition & 0 deletions models/migrations/migrations.go
Original file line number Diff line number Diff line change
Expand Up @@ -386,6 +386,7 @@ func prepareMigrationTasks() []*migration {

// Gitea 1.24.0 ends at database version 321
newMigration(321, "Use LONGTEXT for some columns and fix review_state.updated_files column", v1_25.UseLongTextInSomeColumnsAndFixBugs),
newMigration(322, "Add BeforeCommitID to Comment table", v1_25.AddBeforeCommitIDForComment),
}
return preparedMigrations
}
Expand Down
28 changes: 28 additions & 0 deletions models/migrations/v1_25/v322.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// Copyright 2025 The Gitea Authors. All rights reserved.
// SPDX-License-Identifier: MIT

package v1_25

import (
"xorm.io/xorm"
)

type comment struct {
BeforeCommitID string `xorm:"VARCHAR(64)"`
}

// TableName return database table name for xorm
func (comment) TableName() string {
return "comment"
}

func AddBeforeCommitIDForComment(x *xorm.Engine) error {
if _, err := x.SyncWithOptions(xorm.SyncOptions{
IgnoreConstrains: true,
IgnoreIndices: true,
}, new(comment)); err != nil {
return err
}
_, err := x.Exec("UPDATE comment SET before_commit_id = (SELECT merge_base FROM pull_request WHERE pull_request.issue_id = comment.issue_id) WHERE `type`=21 AND before_commit_id IS NULL")
return err
}
Loading