github/gitea
1
0
Fork 0
mirror of https://github.com/go-gitea/gitea.git synced 2025-08-13 09:30:47 +00:00
Code Issues Packages Projects Releases Wiki Activity

Fix bug

Browse Source
This commit is contained in:
Lunny Xiao 2025-07-22 19:13:13 -07:00
parent 1556911468
commit 8b4e077bdd
No known key found for this signature in database
GPG Key ID: C3B7C91B632F738A
9 changed files with 95 additions and 112 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
models/issues/comment.go
View File

@ -238,6 +238,7 @@ type CommentMetaData struct {
ProjectColumnID int64 `json:"project_column_id,omitempty"` ProjectColumnID int64 `json:"project_column_id,omitempty"`
ProjectColumnTitle string `json:"project_column_title,omitempty"` ProjectColumnTitle string `json:"project_column_title,omitempty"`
ProjectTitle string `json:"project_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. // Comment represents a comment in commit and issue page.
@ -293,7 +294,8 @@ type Comment struct {
UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"` UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"`
// Reference issue in commit message // 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:"-"` Attachments []*repo_model.Attachment `xorm:"-"`
Reactions ReactionList `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()) 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) 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, AssigneeID: opts.AssigneeID,
AssigneeTeamID: opts.AssigneeTeamID, AssigneeTeamID: opts.AssigneeTeamID,
CommitID: opts.CommitID, CommitID: opts.CommitID,
BeforeCommitID: opts.BeforeCommitID,
CommitSHA: opts.CommitSHA, CommitSHA: opts.CommitSHA,
Line: opts.LineNum, Line: opts.LineNum,
Content: opts.Content, Content: opts.Content,
@ -977,7 +980,8 @@ type CreateCommentOptions struct {
OldRef string OldRef string
NewRef string NewRef string
CommitID int64 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 Patch string
LineNum int64 LineNum int64
TreePath string TreePath string

4
models/migrations/migrations.go
View File

@ -24,6 +24,7 @@ import (
"code.gitea.io/gitea/models/migrations/v1_22" "code.gitea.io/gitea/models/migrations/v1_22"
"code.gitea.io/gitea/models/migrations/v1_23" "code.gitea.io/gitea/models/migrations/v1_23"
"code.gitea.io/gitea/models/migrations/v1_24" "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_6"
"code.gitea.io/gitea/models/migrations/v1_7" "code.gitea.io/gitea/models/migrations/v1_7"
"code.gitea.io/gitea/models/migrations/v1_8" "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(318, "Add anonymous_access_mode for repo_unit", v1_24.AddRepoUnitAnonymousAccessMode),
newMigration(319, "Add ExclusiveOrder to Label table", v1_24.AddExclusiveOrderColumnToLabelTable), newMigration(319, "Add ExclusiveOrder to Label table", v1_24.AddExclusiveOrderColumnToLabelTable),
newMigration(320, "Migrate two_factor_policy to login_source table", v1_24.MigrateSkipTwoFactor), 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 return preparedMigrations
} }

4
services/automergequeue/automergequeue.go
View File

@ -39,11 +39,11 @@ func StartPRCheckAndAutoMerge(ctx context.Context, pull *issues_model.PullReques
return return
} }
defer gitRepo.Close() defer gitRepo.Close()
headCommitID, err := gitRepo.GetRefCommitID(pull.GetGitHeadRefName()) commitID, err := gitRepo.GetRefCommitID(pull.GetGitHeadRefName())
if err != nil { if err != nil {
log.Error("GetRefCommitID: %v", err) log.Error("GetRefCommitID: %v", err)
return return
} }
AddToQueue(pull, headCommitID) AddToQueue(pull, commitID)
} }

46
services/doctor/review.go
View File

@ -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,
})
}

8
services/issue/comments.go
View File

@ -153,7 +153,13 @@ func DeleteComment(ctx context.Context, doer *user_model.User, comment *issues_m
} }
if comment.ReviewID > 0 { 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) 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. // 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 // users have to delete this ref manually or we should have a synchronize between

28
services/issue/issue.go
View File

@ -202,22 +202,22 @@ func DeleteIssue(ctx context.Context, doer *user_model.User, gitRepo *git.Reposi
// delete pull request related git data // delete pull request related git data
if issue.IsPull && gitRepo != nil { if issue.IsPull && gitRepo != nil {
if err := git.RemoveRef(ctx, gitRepo.Path, issue.PullRequest.GetGitHeadRefName()); err != 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 issue.PullRequest.HasMerged {
if err := git.RemoveRef(ctx, gitRepo.Path, issue.PullRequest.GetGitMergeRefName()); err != nil { 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 { for _, comment := range comments {
if comment.ReviewID > 0 { if comment.Type == issues_model.CommentTypeCode {
if err := git.RemoveRef(ctx, issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID)); err != nil { 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 in base repository for PR[%d] Error: %v", issue.PullRequest.ID, err) 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)
// 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 if err := git.RemoveRef(ctx, issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID, "after")); err != nil {
// database comment table and git refs. 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 { for _, comment := range comments {
if comment.ReviewID > 0 { if comment.Type == issues_model.CommentTypeCode {
if err := git.RemoveRef(ctx, issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID)); err != nil { 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 in base repository for PR[%d] Error: %v", issue.PullRequest.ID, err) 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)
// 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 if err := git.RemoveRef(ctx, issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID, "after")); err != nil {
// database comment table and git refs. 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)
} }
} }
} }

19
services/migrations/gitea_uploader.go
View File

@ -927,15 +927,16 @@ func (g *GiteaLocalUploader) CreateReviews(ctx context.Context, reviews ...*base
} }
c := issues_model.Comment{ c := issues_model.Comment{
Type: issues_model.CommentTypeCode, Type: issues_model.CommentTypeCode,
IssueID: issue.ID, IssueID: issue.ID,
Content: comment.Content, Content: comment.Content,
Line: int64(line + comment.Position - 1), Line: int64(line + comment.Position - 1),
TreePath: comment.TreePath, TreePath: comment.TreePath,
CommitSHA: comment.CommitID, BeforeCommitID: pr.MergeBase,
Patch: patch, CommitSHA: comment.CommitID,
CreatedUnix: timeutil.TimeStamp(comment.CreatedAt.Unix()), Patch: patch,
UpdatedUnix: timeutil.TimeStamp(comment.UpdatedAt.Unix()), CreatedUnix: timeutil.TimeStamp(comment.CreatedAt.Unix()),
UpdatedUnix: timeutil.TimeStamp(comment.UpdatedAt.Unix()),
} }
if err := g.remapUser(ctx, review, &c); err != nil { if err := g.remapUser(ctx, review, &c); err != nil {

84
services/pull/review.go
View File

@ -268,21 +268,21 @@ func createCodeComment(ctx context.Context, doer *user_model.User, repo *repo_mo
return nil, fmt.Errorf("GetPatch failed: %w", err) 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) { return db.WithTx2(ctx, func(ctx context.Context) (*issues_model.Comment, error) {
comment, err := issues_model.CreateComment(ctx, &issues_model.CreateCommentOptions{ comment, err := issues_model.CreateComment(ctx, &issues_model.CreateCommentOptions{
Type: issues_model.CommentTypeCode, Type: issues_model.CommentTypeCode,
Doer: doer, Doer: doer,
Repo: repo, Repo: repo,
Issue: issue, Issue: issue,
Content: content, Content: content,
LineNum: line, LineNum: line,
TreePath: treePath, TreePath: treePath,
CommitSHA: lineCommitID, BeforeCommitID: beforeCommitID,
ReviewID: reviewID, CommitSHA: afterCommitID,
Patch: patch, ReviewID: reviewID,
Invalidated: false, Patch: patch,
Attachments: attachments, Invalidated: false,
Attachments: attachments,
}) })
if err != nil { if err != nil {
return nil, err 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. // 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 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 { 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 in base repository for PR[%d] Error: %v", pr.ID, err) 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 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. // 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. // If the returned line number is zero, it should not be displayed.
func ReCalculateLineNumber(hunks []*git.HunkInfo, leftLine int64) int64 { func ReCalculateLineNumber(hunks []*git.HunkInfo, leftLine int64) int64 {
if len(hunks) == 0 || leftLine == 0 { if len(hunks) == 0 || leftLine == 0 {
@ -498,15 +503,16 @@ func ReCalculateLineNumber(hunks []*git.HunkInfo, leftLine int64) int64 {
newLine := absLine newLine := absLine
for _, hunk := range hunks { for _, hunk := range hunks {
if hunk.LeftLine+hunk.LeftHunk <= absLine { if absLine < hunk.LeftLine {
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 {
// The line is before the hunk, so we can ignore it // The line is before the hunk, so we can ignore it
continue 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) 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() dstCommitID := beforeCommit.ID.String()
commentCommitID := comment.BeforeCommitID
if comment.Line > 0 { if comment.Line > 0 {
dstCommitID = afterCommit.ID.String() dstCommitID = afterCommit.ID.String()
commentCommitID = comment.CommitSHA
} }
if comment.CommitSHA == dstCommitID { if commentCommitID != dstCommitID {
lineComments[comment.Line] = append(lineComments[comment.Line], comment) // If the comment is not for the current commit, we need to recalculate the line number
continue hunks, ok := hunksCache[commentCommitID+".."+dstCommitID]
} if !ok {
hunks, err = git.GetAffectedHunksForTwoCommitsSpecialFile(ctx, repo.RepoPath(), commentCommitID, dstCommitID, file.Name)
// If the comment is not for the current commit, we need to recalculate the line number if err != nil {
hunks, ok := hunksCache[comment.CommitSHA+".."+dstCommitID] return fmt.Errorf("GetAffectedHunksForTwoCommitsSpecialFile[%s, %s, %s]: %w", repo.FullName(), commentCommitID, dstCommitID, err)
if !ok { }
hunks, err = git.GetAffectedHunksForTwoCommitsSpecialFile(ctx, repo.RepoPath(), comment.CommitSHA, dstCommitID, file.Name) hunksCache[commentCommitID+".."+dstCommitID] = hunks
if err != nil {
return fmt.Errorf("GetAffectedHunksForTwoCommitsSpecialFile[%s, %s, %s]: %w", repo.FullName(), dstCommitID, comment.CommitSHA, err)
} }
hunksCache[comment.CommitSHA+".."+dstCommitID] = hunks comment.Line = ReCalculateLineNumber(hunks, comment.Line)
} }
comment.Line = ReCalculateLineNumber(hunks, comment.Line)
if comment.Line != 0 { 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)
}
} }
} }

4
web_src/js/components/DiffCommitSelector.vue
View File

@ -195,10 +195,10 @@ export default defineComponent({
start = this.commits[firstSelected - 1].id; start = this.commits[firstSelected - 1].id;
} }
const end = this.commits.findLast((x) => x.selected).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 // if the start and end are the same, we show this single commit
window.location.assign(`${this.issueLink}/commits/${start}${this.queryParams}`); 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 // if the first commit is selected and the last commit is selected, we show all commits
window.location.assign(`${this.issueLink}/files${this.queryParams}`); window.location.assign(`${this.issueLink}/files${this.queryParams}`);
} else { } else {