diff --git a/models/issues/comment.go b/models/issues/comment.go index a3a3858a4d5..7f1c2696feb 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -238,6 +238,7 @@ type CommentMetaData struct { ProjectColumnID int64 `json:"project_column_id,omitempty"` ProjectColumnTitle string `json:"project_column_title,omitempty"` ProjectTitle string `json:"project_title,omitempty"` + BeforeCommitID string `json:"before_commit_id,omitempty"` // commit id before this comment } // Comment represents a comment in commit and issue page. @@ -293,7 +294,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:"-"` @@ -764,7 +766,7 @@ func (c *Comment) CodeCommentLink(ctx context.Context) string { return fmt.Sprintf("%s/files#%s", c.Issue.Link(), c.HashTag()) } -func GetCodeCommentRefName(prIndex, commentID int64) string { +func GetCodeCommentRefName(prIndex, commentID int64, suffix string) string { return fmt.Sprintf("refs/pull/%d/code-comment-%d", prIndex, commentID) } @@ -806,6 +808,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, @@ -977,7 +980,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 diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index 176372486e8..e1e35b331c3 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -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" @@ -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-rc0 ends at migration ID number 320 (database version 321) + newMigration(321, "Add BeforeCommitID to Comment table", v1_25.AddBeforeCommitIDForComment), } return preparedMigrations } diff --git a/services/automergequeue/automergequeue.go b/services/automergequeue/automergequeue.go index aed8cded441..fa9c04da874 100644 --- a/services/automergequeue/automergequeue.go +++ b/services/automergequeue/automergequeue.go @@ -39,11 +39,11 @@ func StartPRCheckAndAutoMerge(ctx context.Context, pull *issues_model.PullReques return } defer gitRepo.Close() - headCommitID, err := gitRepo.GetRefCommitID(pull.GetGitHeadRefName()) + commitID, err := gitRepo.GetRefCommitID(pull.GetGitHeadRefName()) if err != nil { log.Error("GetRefCommitID: %v", err) return } - AddToQueue(pull, headCommitID) + AddToQueue(pull, commitID) } diff --git a/services/doctor/review.go b/services/doctor/review.go deleted file mode 100644 index 0f7365b0e3f..00000000000 --- a/services/doctor/review.go +++ /dev/null @@ -1,46 +0,0 @@ -// Copyright 2025 The Gitea Authors. All rights reserved. -// SPDX-License-Identifier: MIT - -package doctor - -import ( - "context" - - "code.gitea.io/gitea/models/db" - "code.gitea.io/gitea/modules/log" -) - -// checkCommitSHAOfPullRequestCodeReviewComment will check if the commit SHA of pull request code review comments -// For a comment with negative line number, it should be the merge base of the pull request if the comment is on the files page -// if it's on a special commit page or a range commit page, it should be the previous commit when reviewing that commit/commit range -// so that this may be broken for those comments submitted in a special commit(non the first one) page or a range commit page -// NOTICE: the fix can only be done once, so it should be run twice or more -func checkCommitSHAOfPullRequestCodeReviewComment(ctx context.Context, logger log.Logger, autofix bool) error { - count, err := db.GetEngine(ctx).SQL("SELECT 1 FROM comment where line < 0 AND commit_sha != (select merge_base from pull_request WHERE issue_id = comment.issue_id)").Count() - if err != nil { - logger.Critical("Error: %v whilst counting wrong comment commit sha", err) - return err - } - if count > 0 { - if autofix { - total, err := db.GetEngine(ctx).Exec("UPDATE comment SET commit_sha = (select merge_base from pull_request WHERE issue_id = comment.issue_id) WHERE line < 0") - if err != nil { - return err - } - logger.Info("%d comments with wrong commit sha fixed\nWARNING: This doctor can only fix this once, so it should NOT be run twice or more", total) - } else { - logger.Warn("%d comments with wrong commit sha exist", count) - } - } - return nil -} - -func init() { - Register(&Check{ - Title: "Check if comment with negative line number has wrong commit sha", - Name: "check-commitsha-review-comment", - IsDefault: true, - Run: checkCommitSHAOfPullRequestCodeReviewComment, - Priority: 3, - }) -} diff --git a/services/issue/comments.go b/services/issue/comments.go index 33934f05d77..1334ff266ce 100644 --- a/services/issue/comments.go +++ b/services/issue/comments.go @@ -153,7 +153,13 @@ func DeleteComment(ctx context.Context, doer *user_model.User, comment *issues_m } if comment.ReviewID > 0 { - if err := git.RemoveRef(ctx, comment.Issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(comment.Issue.PullRequest.Index, comment.ID)); err != nil { + if err := git.RemoveRef(ctx, comment.Issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(comment.Issue.PullRequest.Index, comment.ID, "before")); err != nil { + log.Error("Unable to remove ref in base repository for PR[%d] Error: %v", comment.Issue.PullRequest.ID, err) + // We should not return error here, because the comment has been removed from database. + // users have to delete this ref manually or we should have a synchronize between + // database comment table and git refs. + } + if err := git.RemoveRef(ctx, comment.Issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(comment.Issue.PullRequest.Index, comment.ID, "after")); err != nil { log.Error("Unable to remove ref in base repository for PR[%d] Error: %v", comment.Issue.PullRequest.ID, err) // We should not return error here, because the comment has been removed from database. // users have to delete this ref manually or we should have a synchronize between diff --git a/services/issue/issue.go b/services/issue/issue.go index c1b9a213e28..b1cbcc73989 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -202,22 +202,22 @@ func DeleteIssue(ctx context.Context, doer *user_model.User, gitRepo *git.Reposi // delete pull request related git data if issue.IsPull && gitRepo != nil { if err := git.RemoveRef(ctx, gitRepo.Path, issue.PullRequest.GetGitHeadRefName()); err != nil { - return err + log.Error("Unable to remove ref %s in base repository for PR[%d] Error: %v", issue.PullRequest.GetGitHeadRefName(), issue.PullRequest.ID, err) } if issue.PullRequest.HasMerged { if err := git.RemoveRef(ctx, gitRepo.Path, issue.PullRequest.GetGitMergeRefName()); err != nil { - return err + log.Error("Unable to remove ref %s in base repository for PR[%d] Error: %v", issue.PullRequest.GetGitMergeRefName(), issue.PullRequest.ID, err) } } } for _, comment := range comments { - if comment.ReviewID > 0 { - if err := git.RemoveRef(ctx, issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID)); err != nil { - log.Error("Unable to remove ref in base repository for PR[%d] Error: %v", issue.PullRequest.ID, err) - // We should not return error here, because the comment has been removed from database. - // users have to delete this ref manually or we should have a synchronize between - // database comment table and git refs. + if comment.Type == issues_model.CommentTypeCode { + if err := git.RemoveRef(ctx, issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID, "before")); err != nil { + log.Error("Unable to remove ref %s in base repository for PR[%d] Error: %v", issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID, "before"), issue.PullRequest.ID, err) + } + if err := git.RemoveRef(ctx, issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID, "after")); err != nil { + log.Error("Unable to remove ref %s in base repository for PR[%d] Error: %v", issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID, "after"), issue.PullRequest.ID, err) } } } @@ -431,12 +431,12 @@ func DeleteIssuesByRepoID(ctx context.Context, repoID int64) (attachmentPaths [] } for _, comment := range comments { - if comment.ReviewID > 0 { - if err := git.RemoveRef(ctx, issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID)); err != nil { - log.Error("Unable to remove ref in base repository for PR[%d] Error: %v", issue.PullRequest.ID, err) - // We should not return error here, because the comment has been removed from database. - // users have to delete this ref manually or we should have a synchronize between - // database comment table and git refs. + if comment.Type == issues_model.CommentTypeCode { + if err := git.RemoveRef(ctx, issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID, "before")); err != nil { + log.Error("Unable to remove ref %s in base repository for PR[%d] Error: %v", issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID, "before"), issue.PullRequest.ID, err) + } + if err := git.RemoveRef(ctx, issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID, "after")); err != nil { + log.Error("Unable to remove ref %s in base repository for PR[%d] Error: %v", issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID, "after"), issue.PullRequest.ID, err) } } } diff --git a/services/migrations/gitea_uploader.go b/services/migrations/gitea_uploader.go index 75eb06d01fa..1bf59bb2ab2 100644 --- a/services/migrations/gitea_uploader.go +++ b/services/migrations/gitea_uploader.go @@ -927,15 +927,16 @@ func (g *GiteaLocalUploader) CreateReviews(ctx context.Context, reviews ...*base } c := issues_model.Comment{ - Type: issues_model.CommentTypeCode, - IssueID: issue.ID, - Content: comment.Content, - Line: int64(line + comment.Position - 1), - TreePath: comment.TreePath, - CommitSHA: comment.CommitID, - Patch: patch, - CreatedUnix: timeutil.TimeStamp(comment.CreatedAt.Unix()), - UpdatedUnix: timeutil.TimeStamp(comment.UpdatedAt.Unix()), + Type: issues_model.CommentTypeCode, + IssueID: issue.ID, + Content: comment.Content, + Line: int64(line + comment.Position - 1), + TreePath: comment.TreePath, + BeforeCommitID: pr.MergeBase, + CommitSHA: comment.CommitID, + Patch: patch, + CreatedUnix: timeutil.TimeStamp(comment.CreatedAt.Unix()), + UpdatedUnix: timeutil.TimeStamp(comment.UpdatedAt.Unix()), } if err := g.remapUser(ctx, review, &c); err != nil { diff --git a/services/pull/review.go b/services/pull/review.go index 2fc9c917ac0..401f59fcf9e 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -268,21 +268,21 @@ func createCodeComment(ctx context.Context, doer *user_model.User, repo *repo_mo return nil, fmt.Errorf("GetPatch failed: %w", err) } - lineCommitID := util.Iif(line < 0, beforeCommitID, afterCommitID) return db.WithTx2(ctx, func(ctx context.Context) (*issues_model.Comment, error) { comment, err := issues_model.CreateComment(ctx, &issues_model.CreateCommentOptions{ - Type: issues_model.CommentTypeCode, - Doer: doer, - Repo: repo, - Issue: issue, - Content: content, - LineNum: line, - TreePath: treePath, - CommitSHA: lineCommitID, - ReviewID: reviewID, - Patch: patch, - Invalidated: false, - Attachments: attachments, + Type: issues_model.CommentTypeCode, + Doer: doer, + Repo: repo, + Issue: issue, + Content: content, + LineNum: line, + TreePath: treePath, + BeforeCommitID: beforeCommitID, + CommitSHA: afterCommitID, + ReviewID: reviewID, + Patch: patch, + Invalidated: false, + Attachments: attachments, }) if err != nil { return nil, err @@ -290,8 +290,12 @@ func createCodeComment(ctx context.Context, doer *user_model.User, repo *repo_mo // The line commit ID Must be referenced in the git repository, because the branch maybe rebased or force-pushed. // If the review commit is GC, the position can not be calculated dynamically. - if err := git.UpdateRef(ctx, pr.BaseRepo.RepoPath(), issues_model.GetCodeCommentRefName(pr.Index, comment.ID), lineCommitID); err != nil { - log.Error("Unable to update ref in base repository for PR[%d] Error: %v", pr.ID, err) + if err := git.UpdateRef(ctx, pr.BaseRepo.RepoPath(), issues_model.GetCodeCommentRefName(pr.Index, comment.ID, "before"), beforeCommitID); err != nil { + log.Error("Unable to update ref before_commitid in base repository for PR[%d] Error: %v", pr.ID, err) + return nil, err + } + if err := git.UpdateRef(ctx, pr.BaseRepo.RepoPath(), issues_model.GetCodeCommentRefName(pr.Index, comment.ID, "after"), afterCommitID); err != nil { + log.Error("Unable to update ref after_commitid in base repository for PR[%d] Error: %v", pr.ID, err) return nil, err } @@ -484,6 +488,7 @@ func DismissReview(ctx context.Context, reviewID, repoID int64, message string, } // ReCalculateLineNumber recalculates the line number based on the hunks of the diff. +// left side is the commit the comment was created on, right side is the commit the comment is displayed on. // If the returned line number is zero, it should not be displayed. func ReCalculateLineNumber(hunks []*git.HunkInfo, leftLine int64) int64 { if len(hunks) == 0 || leftLine == 0 { @@ -498,15 +503,16 @@ func ReCalculateLineNumber(hunks []*git.HunkInfo, leftLine int64) int64 { newLine := absLine for _, hunk := range hunks { - if hunk.LeftLine+hunk.LeftHunk <= absLine { - newLine += hunk.RightHunk - hunk.LeftHunk - } else if hunk.LeftLine <= absLine && absLine < hunk.LeftLine+hunk.LeftHunk { - // The line has been removed, so it should not be displayed - return 0 - } else if absLine < hunk.LeftLine { + if absLine < hunk.LeftLine { // The line is before the hunk, so we can ignore it continue + } else if hunk.LeftLine <= absLine && absLine < hunk.LeftLine+hunk.LeftHunk { + // The line is within the hunk, that means the line is deleted from the current commit + // So that we don't need to display this line + return 0 } + // The line is after the hunk, so we can add the right hunk size + newLine += hunk.RightHunk - hunk.LeftHunk } return util.Iif(isLeft, -newLine, newLine) } @@ -614,27 +620,35 @@ func LoadCodeComments(ctx context.Context, gitRepo *git.Repository, repo *repo_m } dstCommitID := beforeCommit.ID.String() + commentCommitID := comment.BeforeCommitID if comment.Line > 0 { dstCommitID = afterCommit.ID.String() + commentCommitID = comment.CommitSHA } - if comment.CommitSHA == dstCommitID { - lineComments[comment.Line] = append(lineComments[comment.Line], comment) - continue - } - - // If the comment is not for the current commit, we need to recalculate the line number - hunks, ok := hunksCache[comment.CommitSHA+".."+dstCommitID] - if !ok { - hunks, err = git.GetAffectedHunksForTwoCommitsSpecialFile(ctx, repo.RepoPath(), comment.CommitSHA, dstCommitID, file.Name) - if err != nil { - return fmt.Errorf("GetAffectedHunksForTwoCommitsSpecialFile[%s, %s, %s]: %w", repo.FullName(), dstCommitID, comment.CommitSHA, err) + if commentCommitID != dstCommitID { + // If the comment is not for the current commit, we need to recalculate the line number + hunks, ok := hunksCache[commentCommitID+".."+dstCommitID] + if !ok { + hunks, err = git.GetAffectedHunksForTwoCommitsSpecialFile(ctx, repo.RepoPath(), commentCommitID, dstCommitID, file.Name) + if err != nil { + return fmt.Errorf("GetAffectedHunksForTwoCommitsSpecialFile[%s, %s, %s]: %w", repo.FullName(), commentCommitID, dstCommitID, err) + } + hunksCache[commentCommitID+".."+dstCommitID] = hunks } - hunksCache[comment.CommitSHA+".."+dstCommitID] = hunks + comment.Line = ReCalculateLineNumber(hunks, comment.Line) } - comment.Line = ReCalculateLineNumber(hunks, comment.Line) + if comment.Line != 0 { - lineComments[comment.Line] = append(lineComments[comment.Line], comment) + commentAfterCommit, err := gitRepo.GetCommit(comment.CommitSHA) + if err != nil { + return fmt.Errorf("GetCommit[%s]: %w", comment.CommitSHA, err) + } + // If the comment is not the first one or the comment created before the current commit + if lineComments[comment.Line] != nil || comment.CommitSHA == afterCommit.ID.String() || + commentAfterCommit.Committer.When.Before(afterCommit.Committer.When) { + lineComments[comment.Line] = append(lineComments[comment.Line], comment) + } } } diff --git a/web_src/js/components/DiffCommitSelector.vue b/web_src/js/components/DiffCommitSelector.vue index 03d55e896b9..71fcb23d2cb 100644 --- a/web_src/js/components/DiffCommitSelector.vue +++ b/web_src/js/components/DiffCommitSelector.vue @@ -195,10 +195,10 @@ export default defineComponent({ start = this.commits[firstSelected - 1].id; } const end = this.commits.findLast((x) => x.selected).id; - if (start === end) { + if (firstSelected === end) { // if the start and end are the same, we show this single commit window.location.assign(`${this.issueLink}/commits/${start}${this.queryParams}`); - } else if (firstSelected === 0 && end === this.commits.at(-1).id) { + } else if (start === this.merge_base && end === this.commits.at(-1).id) { // if the first commit is selected and the last commit is selected, we show all commits window.location.assign(`${this.issueLink}/files${this.queryParams}`); } else {