From 1fb7b013f3c6cdf490723ae58072bfd974a3125e Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 22 Jul 2025 23:10:19 -0700 Subject: [PATCH] improvements --- models/migrations/v1_25/v321.go | 2 +- services/issue/comments.go | 13 ++++----- services/issue/issue.go | 50 +++++++++++++++------------------ services/issue/issue_test.go | 6 ++-- services/pull/review.go | 50 +++++++++++++++++---------------- 5 files changed, 58 insertions(+), 63 deletions(-) diff --git a/models/migrations/v1_25/v321.go b/models/migrations/v1_25/v321.go index 887540d070b..ea5704b5b7a 100644 --- a/models/migrations/v1_25/v321.go +++ b/models/migrations/v1_25/v321.go @@ -23,6 +23,6 @@ func AddBeforeCommitIDForComment(x *xorm.Engine) error { }, 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 before_commit_id IS NULL") + _, 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 } diff --git a/services/issue/comments.go b/services/issue/comments.go index 1334ff266ce..83b6d0b1f47 100644 --- a/services/issue/comments.go +++ b/services/issue/comments.go @@ -134,7 +134,7 @@ func UpdateComment(ctx context.Context, c *issues_model.Comment, contentVersion // DeleteComment deletes the comment func DeleteComment(ctx context.Context, doer *user_model.User, comment *issues_model.Comment) error { - if comment.ReviewID > 0 { + if comment.Type == issues_model.CommentTypeCode { if err := comment.LoadIssue(ctx); err != nil { return err } @@ -152,18 +152,15 @@ func DeleteComment(ctx context.Context, doer *user_model.User, comment *issues_m return err } - if comment.ReviewID > 0 { + if comment.Type == issues_model.CommentTypeCode { + // 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, "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 - // database comment table and git refs. } } diff --git a/services/issue/issue.go b/services/issue/issue.go index b1cbcc73989..2158f5ce861 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -191,7 +191,7 @@ func DeleteIssue(ctx context.Context, doer *user_model.User, gitRepo *git.Reposi } // delete entries in database - attachmentPaths, comments, err := deleteIssue(ctx, issue) + attachmentPaths, err := deleteIssue(ctx, issue) if err != nil { return err } @@ -211,7 +211,7 @@ func DeleteIssue(ctx context.Context, doer *user_model.User, gitRepo *git.Reposi } } - for _, comment := range comments { + for _, comment := range issue.Comments { 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) @@ -277,48 +277,48 @@ func GetRefEndNamesAndURLs(issues []*issues_model.Issue, repoLink string) (map[i } // deleteIssue deletes the issue -func deleteIssue(ctx context.Context, issue *issues_model.Issue) ([]string, []*issues_model.Comment, error) { - var attachmentPaths []string - if err := db.WithTx(ctx, func(ctx context.Context) error { +func deleteIssue(ctx context.Context, issue *issues_model.Issue) ([]string, error) { + return db.WithTx2(ctx, func(ctx context.Context) ([]string, error) { // update the total issue numbers if err := repo_model.UpdateRepoIssueNumbers(ctx, issue.RepoID, issue.IsPull, false); err != nil { - return err + return nil, err } // if the issue is closed, update the closed issue numbers if issue.IsClosed { if err := repo_model.UpdateRepoIssueNumbers(ctx, issue.RepoID, issue.IsPull, true); err != nil { - return err + return nil, err } } if err := issues_model.UpdateMilestoneCounters(ctx, issue.MilestoneID); err != nil { - return fmt.Errorf("error updating counters for milestone id %d: %w", + return nil, fmt.Errorf("error updating counters for milestone id %d: %w", issue.MilestoneID, err) } if err := activities_model.DeleteIssueActions(ctx, issue.RepoID, issue.ID, issue.Index); err != nil { - return err + return nil, err } // find attachments related to this issue and remove them if err := issue.LoadAttachments(ctx); err != nil { - return err + return nil, err } + var attachmentPaths []string for i := range issue.Attachments { attachmentPaths = append(attachmentPaths, issue.Attachments[i].RelativePath()) } // deference all review comments if err := issue.LoadRepo(ctx); err != nil { - return err + return nil, err } if err := issue.LoadPullRequest(ctx); err != nil { - return err + return nil, err } if err := issue.LoadComments(ctx); err != nil { - return err + return nil, err } // delete all database data still assigned to this issue @@ -341,12 +341,12 @@ func deleteIssue(ctx context.Context, issue *issues_model.Issue) ([]string, []*i &issues_model.Comment{DependentIssueID: issue.ID}, &issues_model.IssuePin{IssueID: issue.ID}, ); err != nil { - return err + return nil, err } for _, comment := range issue.Comments { if err := issues_model.DeleteComment(ctx, comment); err != nil { - return err + return nil, err } } @@ -355,29 +355,25 @@ func deleteIssue(ctx context.Context, issue *issues_model.Issue) ([]string, []*i // Delete scheduled auto merges if _, err := db.GetEngine(ctx).Where("pull_id=?", issue.PullRequest.ID). Delete(&pull_model.AutoMerge{}); err != nil { - return err + return nil, err } // Delete review states if _, err := db.GetEngine(ctx).Where("pull_id=?", issue.PullRequest.ID). Delete(&pull_model.ReviewState{}); err != nil { - return err + return nil, err } if _, err := db.GetEngine(ctx).ID(issue.PullRequest.ID).Delete(&issues_model.PullRequest{}); err != nil { - return err + return nil, err } } if _, err := db.GetEngine(ctx).ID(issue.ID).NoAutoCondition().Delete(issue); err != nil { - return err + return nil, err } - return nil - }); err != nil { - return nil, nil, err - } - - return attachmentPaths, issue.Comments, nil + return attachmentPaths, nil + }) } // DeleteOrphanedIssues delete issues without a repo @@ -425,12 +421,12 @@ func DeleteIssuesByRepoID(ctx context.Context, repoID int64) (attachmentPaths [] } for _, issue := range issues { - issueAttachPaths, comments, err := deleteIssue(ctx, issue) + issueAttachPaths, err := deleteIssue(ctx, issue) if err != nil { return nil, fmt.Errorf("deleteIssue [issue_id: %d]: %w", issue.ID, err) } - for _, comment := range comments { + for _, comment := range issue.Comments { 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) diff --git a/services/issue/issue_test.go b/services/issue/issue_test.go index 920a8e5ace7..0527192f916 100644 --- a/services/issue/issue_test.go +++ b/services/issue/issue_test.go @@ -41,7 +41,7 @@ func TestIssue_DeleteIssue(t *testing.T) { issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: issueIDs[2]}) - _, _, err = deleteIssue(db.DefaultContext, issue) + _, err = deleteIssue(db.DefaultContext, issue) assert.NoError(t, err) issueIDs, err = issues_model.GetIssueIDsByRepoID(db.DefaultContext, 1) assert.NoError(t, err) @@ -52,7 +52,7 @@ func TestIssue_DeleteIssue(t *testing.T) { assert.NoError(t, err) issue, err = issues_model.GetIssueByID(db.DefaultContext, 4) assert.NoError(t, err) - _, _, err = deleteIssue(db.DefaultContext, issue) + _, err = deleteIssue(db.DefaultContext, issue) assert.NoError(t, err) assert.Len(t, attachments, 2) for i := range attachments { @@ -75,7 +75,7 @@ func TestIssue_DeleteIssue(t *testing.T) { assert.NoError(t, err) assert.False(t, left) - _, _, err = deleteIssue(db.DefaultContext, issue2) + _, err = deleteIssue(db.DefaultContext, issue2) assert.NoError(t, err) left, err = issues_model.IssueNoDependenciesLeft(db.DefaultContext, issue1) assert.NoError(t, err) diff --git a/services/pull/review.go b/services/pull/review.go index 401f59fcf9e..f1a3550b806 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -518,7 +518,7 @@ func ReCalculateLineNumber(hunks []*git.HunkInfo, leftLine int64) int64 { } // FetchCodeCommentsByLine fetches the code comments for a given commit, treePath and line number of a pull request. -func FetchCodeCommentsByLine(ctx context.Context, gitRepo *git.Repository, repo *repo_model.Repository, issueID int64, currentUser *user_model.User, startCommitID, endCommitID, treePath string, line int64, showOutdatedComments bool) (issues_model.CommentList, error) { +func FetchCodeCommentsByLine(ctx context.Context, gitRepo *git.Repository, repo *repo_model.Repository, issueID int64, currentUser *user_model.User, beforeCommitID, afterCommitID, treePath string, line int64, showOutdatedComments bool) (issues_model.CommentList, error) { opts := issues_model.FindCommentsOptions{ Type: issues_model.CommentTypeCode, IssueID: issueID, @@ -533,52 +533,54 @@ func FetchCodeCommentsByLine(ctx context.Context, gitRepo *git.Repository, repo if len(comments) == 0 { return nil, nil } + afterCommit, err := gitRepo.GetCommit(afterCommitID) + if err != nil { + return nil, fmt.Errorf("GetCommit[%s]: %w", afterCommitID, err) + } n := 0 hunksCache := make(map[string][]*git.HunkInfo) for _, comment := range comments { // Code comment should always have a commit SHA, if not, we need to set it based on the line number if comment.CommitSHA == "" { if comment.Line > 0 { - comment.CommitSHA = endCommitID + comment.CommitSHA = afterCommitID } else if comment.Line < 0 { - comment.CommitSHA = startCommitID + comment.CommitSHA = beforeCommitID } else { // If the comment has no line number, we cannot display it in the diff view continue } } - dstCommitID := startCommitID + dstCommitID := beforeCommitID + commentCommitID := comment.BeforeCommitID if comment.Line > 0 { - dstCommitID = endCommitID + dstCommitID = afterCommitID + commentCommitID = comment.CommitSHA } - if comment.CommitSHA == dstCommitID { - if comment.Line == line { - comments[n] = comment - n++ + 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, treePath) + if err != nil { + return nil, fmt.Errorf("GetAffectedHunksForTwoCommitsSpecialFile[%s, %s, %s]: %w", repo.FullName(), commentCommitID, dstCommitID, err) + } + hunksCache[commentCommitID+".."+dstCommitID] = hunks } - continue + comment.Line = ReCalculateLineNumber(hunks, comment.Line) } - // 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, treePath) + if comment.Line == line { + commentAfterCommit, err := gitRepo.GetCommit(comment.CommitSHA) if err != nil { - return nil, fmt.Errorf("GetAffectedHunksForTwoCommitsSpecialFile[%s, %s, %s]: %w", repo.FullName(), comment.CommitSHA, dstCommitID, err) + return nil, fmt.Errorf("GetCommit[%s]: %w", comment.CommitSHA, err) } - hunksCache[comment.CommitSHA+".."+dstCommitID] = hunks - } - comment.Line = ReCalculateLineNumber(hunks, comment.Line) - if comment.Line != 0 { - dstCommit, err := gitRepo.GetCommit(dstCommitID) - if err != nil { - return nil, fmt.Errorf("GetCommit[%s]: %w", dstCommitID, err) - } // If the comment is not the first one or the comment created before the current commit - if n > 0 || comment.CreatedUnix.AsTime().Before(dstCommit.Committer.When) { + if n > 0 || comment.CommitSHA == afterCommitID || + commentAfterCommit.Committer.When.Before(afterCommit.Committer.When) { comments[n] = comment n++ }