From f4304547a458b969668f19b4b369db31f6e88831 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 16 Feb 2026 21:42:34 -0800 Subject: [PATCH 1/6] Fix force push comments of pull request --- services/pull/comment.go | 61 ++++++++++++++++------- services/pull/comment_test.go | 94 +++++++++++++++++++++++++++++++++++ 2 files changed, 137 insertions(+), 18 deletions(-) create mode 100644 services/pull/comment_test.go diff --git a/services/pull/comment.go b/services/pull/comment.go index 6c10bf2aa8..31ecd95019 100644 --- a/services/pull/comment.go +++ b/services/pull/comment.go @@ -6,6 +6,7 @@ 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" @@ -61,27 +62,51 @@ func CreatePushPullComment(ctx context.Context, pusher *user_model.User, pr *iss } 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 { - 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 - } - } - - dataJSON, err := json.Marshal(data) + data.CommitIDs, err = getCommitIDsFromRepo(ctx, pr.BaseRepo, oldCommitID, newCommitID, pr.BaseBranch) if err != nil { return nil, err } + // It maybe an empty pull request. Only non-empty pull request need to create push comment + if len(data.CommitIDs) == 0 && !isForcePush { + return nil, nil //nolint:nilnil // return nil because no comment needs to be created + } - opts.Content = string(dataJSON) - comment, err = issues_model.CreateComment(ctx, opts) + 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 + } + } - return comment, 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 + } + } + + if opts.IsForcePush { // if it's a force push, we needs 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) + comment, err = issues_model.CreateComment(ctx, opts) + if err != nil { + return nil, err + } + } + + return comment, err + }) } diff --git a/services/pull/comment_test.go b/services/pull/comment_test.go new file mode 100644 index 0000000000..a91bb6f00d --- /dev/null +++ b/services/pull/comment_test.go @@ -0,0 +1,94 @@ +// 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) { + 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) + + commits, err := gitRepo.CommitsBetweenNotBase(headCommit, oldCommit, pr.BaseBranch) + assert.NoError(t, err) + expectedCount := 1 + if len(commits) > 0 { + expectedCount = 2 + } + + 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, expectedCount) + + 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) +} From a598a4466b6ff7750d4010a91c2f29ccbc6b5bf8 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 17 Feb 2026 20:07:34 -0800 Subject: [PATCH 2/6] Fix --- services/pull/comment.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/services/pull/comment.go b/services/pull/comment.go index 31ecd95019..0ad0419441 100644 --- a/services/pull/comment.go +++ b/services/pull/comment.go @@ -54,11 +54,10 @@ 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 @@ -93,7 +92,7 @@ func CreatePushPullComment(ctx context.Context, pusher *user_model.User, pr *iss } } - if opts.IsForcePush { // if it's a force push, we needs to add a force push comment + if isForcePush { // if it's a force push, we needs to add a force push comment data.CommitIDs = []string{oldCommitID, newCommitID} data.IsForcePush = true dataJSON, err := json.Marshal(data) @@ -101,6 +100,7 @@ func CreatePushPullComment(ctx context.Context, pusher *user_model.User, pr *iss 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 From 2a6c3e70f9091c860a0aada8c8221491cabec39a Mon Sep 17 00:00:00 2001 From: silverwind Date: Wed, 18 Feb 2026 05:29:47 +0100 Subject: [PATCH 3/6] Update services/pull/comment.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: silverwind --- services/pull/comment.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/pull/comment.go b/services/pull/comment.go index 0ad0419441..c6a4ea5d50 100644 --- a/services/pull/comment.go +++ b/services/pull/comment.go @@ -92,7 +92,7 @@ func CreatePushPullComment(ctx context.Context, pusher *user_model.User, pr *iss } } - if isForcePush { // if it's a force push, we needs to add a force push comment + 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) From 02241a94dfc6261479ed5b1f81b1f12ff35e51ed Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 19 Feb 2026 11:48:22 -0800 Subject: [PATCH 4/6] Update services/pull/comment_test.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Lunny Xiao --- services/pull/comment_test.go | 213 +++++++++++++++++++++++----------- 1 file changed, 146 insertions(+), 67 deletions(-) diff --git a/services/pull/comment_test.go b/services/pull/comment_test.go index a91bb6f00d..a01fe3c524 100644 --- a/services/pull/comment_test.go +++ b/services/pull/comment_test.go @@ -16,79 +16,158 @@ import ( ) func TestCreatePushPullCommentForcePushDeletesOldComments(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) + 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())) + 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}) + 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) + _, 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) - 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) + 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) - commits, err := gitRepo.CommitsBetweenNotBase(headCommit, oldCommit, pr.BaseBranch) - assert.NoError(t, err) - expectedCount := 1 - if len(commits) > 0 { - expectedCount = 2 - } + gitRepo, err := gitrepo.OpenRepository(t.Context(), pr.BaseRepo) + assert.NoError(t, err) + defer gitRepo.Close() - 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, expectedCount) - - forcePushCount := 0 - for _, comment := range comments { - var pushData issues_model.PushActionContent - assert.NoError(t, json.Unmarshal([]byte(comment.Content), &pushData)) - if pushData.IsForcePush { - forcePushCount++ + 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 } - } - assert.Equal(t, 1, forcePushCount) + + 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.Len(t, commits, 0) + + 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.Greater(t, len(commits), 0) + + 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) + }) } From 479370e5c9e588119ab222cd12fa619620f734b3 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 19 Feb 2026 12:06:38 -0800 Subject: [PATCH 5/6] don't return if get force push commits failure --- services/pull/comment.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/services/pull/comment.go b/services/pull/comment.go index 0ad0419441..1c86517afb 100644 --- a/services/pull/comment.go +++ b/services/pull/comment.go @@ -12,6 +12,7 @@ import ( 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 @@ -63,9 +64,15 @@ func CreatePushPullComment(ctx context.Context, pusher *user_model.User, pr *iss var data issues_model.PushActionContent data.CommitIDs, err = getCommitIDsFromRepo(ctx, pr.BaseRepo, oldCommitID, newCommitID, pr.BaseBranch) if err != nil { - return nil, err + // 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 + } + 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 } From 5fd70c971f87134fa1b82ede7d436359e77bc657 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 19 Feb 2026 13:08:06 -0800 Subject: [PATCH 6/6] Fix lint --- services/pull/comment_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/services/pull/comment_test.go b/services/pull/comment_test.go index a01fe3c524..580c1bb0b1 100644 --- a/services/pull/comment_test.go +++ b/services/pull/comment_test.go @@ -73,7 +73,7 @@ func TestCreatePushPullCommentForcePushDeletesOldComments(t *testing.T) { // 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.Len(t, commits, 0) + assert.Empty(t, commits) comments, err = issues_model.FindComments(t.Context(), &issues_model.FindCommentsOptions{ IssueID: pr.IssueID, @@ -150,7 +150,7 @@ func TestCreatePushPullCommentForcePushDeletesOldComments(t *testing.T) { 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.Greater(t, len(commits), 0) + assert.NotEmpty(t, commits) comments, err = issues_model.FindComments(t.Context(), &issues_model.FindCommentsOptions{ IssueID: pr.IssueID,