Skip to content

Improve code review comments #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

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from
Draft
17 changes: 17 additions & 0 deletions models/issues/comment.go
Original file line number Diff line number Diff line change
Expand Up @@ -764,6 +764,10 @@ func (c *Comment) CodeCommentLink(ctx context.Context) string {
return fmt.Sprintf("%s/files#%s", c.Issue.Link(), c.HashTag())
}

func GetCodeCommentRef(prIndex, commentID int64) 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) {
ctx, committer, err := db.TxContext(ctx)
Expand Down Expand Up @@ -1007,6 +1011,7 @@ type FindCommentsOptions struct {
RepoID int64
IssueID int64
ReviewID int64
CommitSHA string
Since int64
Before int64
Line int64
Expand Down Expand Up @@ -1052,6 +1057,9 @@ func (opts FindCommentsOptions) ToConds() builder.Cond {
if opts.IsPull.Has() {
cond = cond.And(builder.Eq{"issue.is_pull": opts.IsPull.Value()})
}
if opts.CommitSHA != "" {
cond = cond.And(builder.Eq{"comment.commit_sha": opts.CommitSHA})
}
return cond
}

Expand Down Expand Up @@ -1147,6 +1155,15 @@ func DeleteComment(ctx context.Context, comment *Comment) error {
return err
}

// delete review if the comment is the last comment of the review
if comment.ReviewID > 0 {
if _, err := db.GetEngine(ctx).ID(comment.ReviewID).
Where("NOT EXISTS (SELECT 1 FROM comment WHERE review_id = ?)", comment.ReviewID).
Delete(new(Review)); err != nil {
return err
}
}

if err := comment.neuterCrossReferences(ctx); err != nil {
return err
}
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 @@ -413,11 +392,16 @@ func (pr *PullRequest) getReviewedByLines(ctx context.Context, writer io.Writer)
return committer.Commit()
}

// GetGitRefName 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 @@ -432,6 +432,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
4 changes: 4 additions & 0 deletions models/migrations/migrations.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"code.gitea.io/gitea/models/migrations/v1_22"
"code.gitea.io/gitea/models/migrations/v1_23"
"code.gitea.io/gitea/models/migrations/v1_24"
"code.gitea.io/gitea/models/migrations/v1_25"
"code.gitea.io/gitea/models/migrations/v1_6"
"code.gitea.io/gitea/models/migrations/v1_7"
"code.gitea.io/gitea/models/migrations/v1_8"
Expand Down Expand Up @@ -382,6 +383,9 @@ func prepareMigrationTasks() []*migration {
newMigration(318, "Add anonymous_access_mode for repo_unit", v1_24.AddRepoUnitAnonymousAccessMode),
newMigration(319, "Add ExclusiveOrder to Label table", v1_24.AddExclusiveOrderColumnToLabelTable),
newMigration(320, "Migrate two_factor_policy to login_source table", v1_24.MigrateSkipTwoFactor),

// Gitea 1.24.0 ends at database version 321
newMigration(321, "Migrate commit id of pull requests code review comment", v1_25.MigrateCommitIDOfPullRequestCodeReviewComment),
}
return preparedMigrations
}
Expand Down
14 changes: 14 additions & 0 deletions models/migrations/v1_25/v321.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// Copyright 2025 The Gitea Authors. All rights reserved.
// SPDX-License-Identifier: MIT

package v1_25

import (
"xorm.io/xorm"
)

// MigrateCommitIDOfPullRequestCodeReviewComment this will be almost right before comment on the special commit of the pull request
func MigrateCommitIDOfPullRequestCodeReviewComment(x *xorm.Engine) error {
_, err := x.Exec("UPDATE comment SET commit_sha = (select merge_base from pull_request WHERE issue_id = comment.issue_id) WHERE line < 0")
return err
}
6 changes: 1 addition & 5 deletions modules/git/commit.go
Original file line number Diff line number Diff line change
Expand Up @@ -258,11 +258,7 @@ func (c *Commit) CommitsBeforeLimit(num int) ([]*Commit, error) {

// CommitsBeforeUntil returns the commits between commitID to current revision
func (c *Commit) CommitsBeforeUntil(commitID string) ([]*Commit, error) {
endCommit, err := c.repo.GetCommit(commitID)
if err != nil {
return nil, err
}
return c.repo.CommitsBetween(c, endCommit)
return c.repo.CommitsBetween(c.ID.String(), commitID)
}

// SearchCommitsOptions specify the parameters for SearchCommits
Expand Down
56 changes: 56 additions & 0 deletions modules/git/diff.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,12 +107,16 @@ func ParseDiffHunkString(diffHunk string) (leftLine, leftHunk, rightLine, rightH
leftLine, _ = strconv.Atoi(leftRange[0][1:])
if len(leftRange) > 1 {
leftHunk, _ = strconv.Atoi(leftRange[1])
} else {
leftHunk = 1
}
if len(ranges) > 1 {
rightRange := strings.Split(ranges[1], ",")
rightLine, _ = strconv.Atoi(rightRange[0])
if len(rightRange) > 1 {
rightHunk, _ = strconv.Atoi(rightRange[1])
} else {
rightHunk = 1
}
} else {
log.Debug("Parse line number failed: %v", diffHunk)
Expand Down Expand Up @@ -342,3 +346,55 @@ func GetAffectedFiles(repo *Repository, branchName, oldCommitID, newCommitID str

return affectedFiles, err
}

type HunkInfo struct {
LeftLine int64 // Line number in the old file
LeftHunk int64 // Number of lines in the old file
RightLine int64 // Line number in the new file
RightHunk int64 // Number of lines in the new file
}

// GetAffectedHunksForTwoCommitsSpecialFile returns the affected hunks between two commits for a special file
// git diff --unified=0 abc123 def456 -- src/main.go
func GetAffectedHunksForTwoCommitsSpecialFile(ctx context.Context, repoPath, oldCommitID, newCommitID, filePath string) ([]*HunkInfo, error) {
reader, writer := io.Pipe()
defer func() {
_ = reader.Close()
_ = writer.Close()
}()
go func() {
if err := NewCommand("diff", "--unified=0", "--no-color").
AddDynamicArguments(oldCommitID, newCommitID).
AddDashesAndList(filePath).
Run(ctx, &RunOpts{
Dir: repoPath,
Stdout: writer,
}); err != nil {
_ = writer.CloseWithError(fmt.Errorf("GetAffectedHunksForTwoCommitsSpecialFile[%s, %s, %s, %s]: %w", repoPath, oldCommitID, newCommitID, filePath, err))
return
}
_ = writer.Close()
}()

scanner := bufio.NewScanner(reader)
hunks := make([]*HunkInfo, 0, 32)
for scanner.Scan() {
lof := scanner.Text()
if !strings.HasPrefix(lof, "@@") {
continue
}
// Parse the hunk header
leftLine, leftHunk, rightLine, rightHunk := ParseDiffHunkString(lof)
hunks = append([]*HunkInfo{}, &HunkInfo{
LeftLine: int64(leftLine),
LeftHunk: int64(leftHunk),
RightLine: int64(rightLine),
RightHunk: int64(rightHunk),
})
}
if scanner.Err() != nil {
return nil, fmt.Errorf("GetAffectedHunksForTwoCommitsSpecialFile[%s, %s, %s, %s]: %w", repoPath, oldCommitID, newCommitID, filePath, scanner.Err())
}

return hunks, nil
}
Loading