diff --git a/services/pull/comment.go b/services/pull/comment.go index 6c10bf2aa8..dec67f57e9 100644 --- a/services/pull/comment.go +++ b/services/pull/comment.go @@ -6,11 +6,13 @@ package pull import ( "context" + "code.gitea.io/gitea/models/db" issues_model "code.gitea.io/gitea/models/issues" repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/json" + "code.gitea.io/gitea/modules/log" ) // getCommitIDsFromRepo get commit IDs from repo in between oldCommitID and newCommitID @@ -53,35 +55,65 @@ func CreatePushPullComment(ctx context.Context, pusher *user_model.User, pr *iss } opts := &issues_model.CreateCommentOptions{ - Type: issues_model.CommentTypePullRequestPush, - Doer: pusher, - Repo: pr.BaseRepo, - IsForcePush: isForcePush, - Issue: pr.Issue, + Type: issues_model.CommentTypePullRequestPush, + Doer: pusher, + Repo: pr.BaseRepo, + Issue: pr.Issue, } var data issues_model.PushActionContent - if opts.IsForcePush { - data.CommitIDs = []string{oldCommitID, newCommitID} - data.IsForcePush = true - } else { - data.CommitIDs, err = getCommitIDsFromRepo(ctx, pr.BaseRepo, oldCommitID, newCommitID, pr.BaseBranch) - if err != nil { + data.CommitIDs, err = getCommitIDsFromRepo(ctx, pr.BaseRepo, oldCommitID, newCommitID, pr.BaseBranch) + if err != nil { + // For force-push events, a missing/unreachable old commit should not prevent + // deleting stale push comments or creating the force-push timeline entry. + if !isForcePush { return nil, err } - // It maybe an empty pull request. Only non-empty pull request need to create push comment - if len(data.CommitIDs) == 0 { - return nil, nil //nolint:nilnil // return nil because no comment needs to be created + log.Error("getCommitIDsFromRepo: %v", err) + } + // It maybe an empty pull request. Only non-empty pull request need to create push comment + // for force push, we always need to delete the old push comment so don't return here. + if len(data.CommitIDs) == 0 && !isForcePush { + return nil, nil //nolint:nilnil // return nil because no comment needs to be created + } + + return db.WithTx2(ctx, func(ctx context.Context) (*issues_model.Comment, error) { + if isForcePush { + if _, err := db.GetEngine(ctx).Where("issue_id = ?", pr.IssueID). + And("type = ?", issues_model.CommentTypePullRequestPush). + NoAutoCondition(). + Delete(new(issues_model.Comment)); err != nil { + return nil, err + } } - } - dataJSON, err := json.Marshal(data) - if err != nil { - return nil, err - } + if len(data.CommitIDs) > 0 { + dataJSON, err := json.Marshal(data) + if err != nil { + return nil, err + } + opts.Content = string(dataJSON) + comment, err = issues_model.CreateComment(ctx, opts) + if err != nil { + return nil, err + } + } - opts.Content = string(dataJSON) - comment, err = issues_model.CreateComment(ctx, opts) + if isForcePush { // if it's a force push, we need to add a force push comment + data.CommitIDs = []string{oldCommitID, newCommitID} + data.IsForcePush = true + dataJSON, err := json.Marshal(data) + if err != nil { + return nil, err + } + opts.Content = string(dataJSON) + opts.IsForcePush = true // FIXME: it seems the field is unnecessary any more because PushActionContent includes IsForcePush field + comment, err = issues_model.CreateComment(ctx, opts) + if err != nil { + return nil, err + } + } - return comment, err + return comment, err + }) } diff --git a/services/pull/comment_test.go b/services/pull/comment_test.go new file mode 100644 index 0000000000..580c1bb0b1 --- /dev/null +++ b/services/pull/comment_test.go @@ -0,0 +1,173 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package pull + +import ( + "testing" + + issues_model "code.gitea.io/gitea/models/issues" + "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/gitrepo" + "code.gitea.io/gitea/modules/json" + + "github.com/stretchr/testify/assert" +) + +func TestCreatePushPullCommentForcePushDeletesOldComments(t *testing.T) { + t.Run("base-branch-only", func(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2}) + assert.NoError(t, pr.LoadIssue(t.Context())) + assert.NoError(t, pr.LoadBaseRepo(t.Context())) + + pusher := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + + _, err := issues_model.CreateComment(t.Context(), &issues_model.CreateCommentOptions{ + Type: issues_model.CommentTypePullRequestPush, + Doer: pusher, + Repo: pr.BaseRepo, + Issue: pr.Issue, + Content: "{}", + }) + assert.NoError(t, err) + _, err = issues_model.CreateComment(t.Context(), &issues_model.CreateCommentOptions{ + Type: issues_model.CommentTypePullRequestPush, + Doer: pusher, + Repo: pr.BaseRepo, + Issue: pr.Issue, + Content: "{}", + }) + assert.NoError(t, err) + + comments, err := issues_model.FindComments(t.Context(), &issues_model.FindCommentsOptions{ + IssueID: pr.IssueID, + Type: issues_model.CommentTypePullRequestPush, + }) + assert.NoError(t, err) + assert.Len(t, comments, 2) + + gitRepo, err := gitrepo.OpenRepository(t.Context(), pr.BaseRepo) + assert.NoError(t, err) + defer gitRepo.Close() + + headCommit, err := gitRepo.GetBranchCommit(pr.BaseBranch) + assert.NoError(t, err) + oldCommit := headCommit + if headCommit.ParentCount() > 0 { + parentCommit, err := headCommit.Parent(0) + assert.NoError(t, err) + oldCommit = parentCommit + } + + comment, err := CreatePushPullComment(t.Context(), pusher, pr, oldCommit.ID.String(), headCommit.ID.String(), true) + assert.NoError(t, err) + assert.NotNil(t, comment) + var createdData issues_model.PushActionContent + assert.NoError(t, json.Unmarshal([]byte(comment.Content), &createdData)) + assert.True(t, createdData.IsForcePush) + + // When both commits are on the base branch, CommitsBetweenNotBase should + // typically return no commits, so only the force-push comment is expected. + commits, err := gitRepo.CommitsBetweenNotBase(headCommit, oldCommit, pr.BaseBranch) + assert.NoError(t, err) + assert.Empty(t, commits) + + comments, err = issues_model.FindComments(t.Context(), &issues_model.FindCommentsOptions{ + IssueID: pr.IssueID, + Type: issues_model.CommentTypePullRequestPush, + }) + assert.NoError(t, err) + assert.Len(t, comments, 1) + + forcePushCount := 0 + for _, comment := range comments { + var pushData issues_model.PushActionContent + assert.NoError(t, json.Unmarshal([]byte(comment.Content), &pushData)) + if pushData.IsForcePush { + forcePushCount++ + } + } + assert.Equal(t, 1, forcePushCount) + }) + + t.Run("head-vs-base-branch", func(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2}) + assert.NoError(t, pr.LoadIssue(t.Context())) + assert.NoError(t, pr.LoadBaseRepo(t.Context())) + + pusher := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + + _, err := issues_model.CreateComment(t.Context(), &issues_model.CreateCommentOptions{ + Type: issues_model.CommentTypePullRequestPush, + Doer: pusher, + Repo: pr.BaseRepo, + Issue: pr.Issue, + Content: "{}", + }) + assert.NoError(t, err) + _, err = issues_model.CreateComment(t.Context(), &issues_model.CreateCommentOptions{ + Type: issues_model.CommentTypePullRequestPush, + Doer: pusher, + Repo: pr.BaseRepo, + Issue: pr.Issue, + Content: "{}", + }) + assert.NoError(t, err) + + comments, err := issues_model.FindComments(t.Context(), &issues_model.FindCommentsOptions{ + IssueID: pr.IssueID, + Type: issues_model.CommentTypePullRequestPush, + }) + assert.NoError(t, err) + assert.Len(t, comments, 2) + + gitRepo, err := gitrepo.OpenRepository(t.Context(), pr.BaseRepo) + assert.NoError(t, err) + defer gitRepo.Close() + + // In this subtest, use the head branch for the new commit and the base branch + // for the old commit so that CommitsBetweenNotBase returns non-empty results. + headCommit, err := gitRepo.GetBranchCommit(pr.HeadBranch) + assert.NoError(t, err) + + baseCommit, err := gitRepo.GetBranchCommit(pr.BaseBranch) + assert.NoError(t, err) + oldCommit := baseCommit + + comment, err := CreatePushPullComment(t.Context(), pusher, pr, oldCommit.ID.String(), headCommit.ID.String(), true) + assert.NoError(t, err) + assert.NotNil(t, comment) + var createdData issues_model.PushActionContent + assert.NoError(t, json.Unmarshal([]byte(comment.Content), &createdData)) + assert.True(t, createdData.IsForcePush) + + commits, err := gitRepo.CommitsBetweenNotBase(headCommit, oldCommit, pr.BaseBranch) + assert.NoError(t, err) + // For this scenario we expect at least one commit between head and base + // that is not on the base branch, so data.CommitIDs should be non-empty. + assert.NotEmpty(t, commits) + + comments, err = issues_model.FindComments(t.Context(), &issues_model.FindCommentsOptions{ + IssueID: pr.IssueID, + Type: issues_model.CommentTypePullRequestPush, + }) + assert.NoError(t, err) + // Two comments should exist now: one regular push comment and one force-push comment. + assert.Len(t, comments, 2) + + forcePushCount := 0 + for _, comment := range comments { + var pushData issues_model.PushActionContent + assert.NoError(t, json.Unmarshal([]byte(comment.Content), &pushData)) + if pushData.IsForcePush { + forcePushCount++ + } + } + assert.Equal(t, 1, forcePushCount) + }) +}