From 6bc0e8cd575d470d6453cb0a8661f91a8d2cf26c Mon Sep 17 00:00:00 2001 From: Giteabot Date: Sun, 18 Oct 2026 03:24:11 -0700 Subject: [PATCH] fix(pull): recover PRs whose recorded merge commit never reached the base repo (#39831) (#39840) Backport #39831 by @lunny When pushing the merge commit to the base repo fails (e.g. non-fast-forward), the merge intent recorded by https://github.com/go-gitea/gitea/pull/39442 is left behind. `merged_commit_id` then points to a commit that does not exist in the base repo, and `git branch --contains` fails with `no such commit`, so the PR can never be checked or merged again. - Clear the recorded merge intent if the push fails and the commit is not on the base branch - `IsCommitInBranch` now returns false for a missing commit, so already-stuck PRs recover on the next check Fixes: https://github.com/go-gitea/gitea/issues/39824 Co-authored-by: Lunny Xiao Co-authored-by: silverwind --- modules/git/repo_commit.go | 22 ++++++++++++++++++++-- modules/git/repo_commit_test.go | 18 ++++++++++++++++++ services/pull/merge.go | 19 +++++++++++++++++++ services/pull/merge_test.go | 20 ++++++++++++++++++++ 4 files changed, 77 insertions(+), 2 deletions(-) diff --git a/modules/git/repo_commit.go b/modules/git/repo_commit.go index 54f5fb76719..1a7547a13d8 100644 --- a/modules/git/repo_commit.go +++ b/modules/git/repo_commit.go @@ -415,8 +415,26 @@ func (repo *Repository) GetCommitsFromIDs(ctx context.Context, commitIDs []strin return commits } -// IsCommitInBranch check if the commit is on the branch +// IsCommitInRepo checks if the commit exists in the repository. +func IsCommitInRepo(ctx context.Context, repo gitrepo.RepositoryFacade, commitID string) (bool, error) { + // "--quiet" makes it exit with 1 and no output if the commit doesn't exist + _, _, err := gitcmd.NewCommand("rev-parse", "--verify", "--quiet"). + AddDynamicArguments(commitID + "^{commit}"). + WithRepo(repo). + RunStdString(ctx) + if gitcmd.IsErrorExitCode(err, 1) { + return false, nil + } + return err == nil, err +} + +// IsCommitInBranch checks if the commit is on the branch. +// It returns false (no error) if the commit doesn't exist or isn't on the branch. func IsCommitInBranch(ctx context.Context, repo gitrepo.RepositoryFacade, commitID, branch string) (r bool, err error) { + if exist, err := IsCommitInRepo(ctx, repo, commitID); err != nil || !exist { + return false, err + } + stdout, _, err := gitcmd.NewCommand("branch", "--contains"). AddDynamicArguments(commitID, branch). WithRepo(repo). @@ -424,7 +442,7 @@ func IsCommitInBranch(ctx context.Context, repo gitrepo.RepositoryFacade, commit if err != nil { return false, err } - return len(stdout) > 0, err + return len(stdout) > 0, nil } // GetCommitBranchStart returns the commit where the branch diverged diff --git a/modules/git/repo_commit_test.go b/modules/git/repo_commit_test.go index 3d0f07f0a3c..b89ad03ad27 100644 --- a/modules/git/repo_commit_test.go +++ b/modules/git/repo_commit_test.go @@ -70,6 +70,20 @@ func TestGetCommitWithBadCommitID(t *testing.T) { assert.True(t, IsErrNotExist(err)) } +func TestIsCommitInRepo(t *testing.T) { + bareRepo1, err := OpenRepositoryLocal(t.Context(), filepath.Join(testReposDir, "repo1_bare")) + assert.NoError(t, err) + defer bareRepo1.Close() + + exist, err := IsCommitInRepo(t.Context(), bareRepo1, "2839944139e0de9737a044f78b0e4b40d989a9e3") + assert.NoError(t, err) + assert.True(t, exist) + + exist, err = IsCommitInRepo(t.Context(), bareRepo1, "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa") + assert.NoError(t, err) + assert.False(t, exist) +} + func TestIsCommitInBranch(t *testing.T) { bareRepo1Path := filepath.Join(testReposDir, "repo1_bare") bareRepo1, err := OpenRepositoryLocal(t.Context(), bareRepo1Path) @@ -83,6 +97,10 @@ func TestIsCommitInBranch(t *testing.T) { result, err = IsCommitInBranch(t.Context(), bareRepo1, "2839944139e0de9737a044f78b0e4b40d989a9e3", "branch2") assert.NoError(t, err) assert.False(t, result) + + result, err = IsCommitInBranch(t.Context(), bareRepo1, "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", "branch1") + assert.NoError(t, err) + assert.False(t, result) } func TestRepository_CommitsBetween(t *testing.T) { diff --git a/services/pull/merge.go b/services/pull/merge.go index cb3e51d9336..21122fda42c 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -279,6 +279,18 @@ func recordMergeIntent(ctx context.Context, pr *issues_model.PullRequest, doer * }) } +// clearMergeIntent reverts recordMergeIntent after the merged commit failed to reach the base repo +func clearMergeIntent(ctx context.Context, pr *issues_model.PullRequest) error { + return db.WithTx(ctx, func(ctx context.Context) error { + pr.MergedUnix, pr.MergedCommitID, pr.MergerID = 0, "", 0 + if _, err := db.GetEngine(ctx).NoAutoTime().ID(pr.ID).Cols("merged_unix", "merged_commit_id", "merger_id").Update(pr); err != nil { + return err + } + _, err := db.GetEngine(ctx).Where("pull_id=?", pr.ID).Cols("merged_commit_id").Update(&pull_model.AutoMerge{}) + return err + }) +} + func hasPullRequestCommitBeenMerged(ctx context.Context, pr *issues_model.PullRequest) (bool, error) { if pr.MergedCommitID == "" { return false, nil @@ -286,6 +298,7 @@ func hasPullRequestCommitBeenMerged(ctx context.Context, pr *issues_model.PullRe if err := pr.LoadBaseRepo(ctx); err != nil { return false, err } + // the recorded commit is missing from the base repo if the push never succeeded, then it is not merged return git.IsCommitInBranch(ctx, pr.BaseRepo, pr.MergedCommitID, pr.BaseBranch) } @@ -340,6 +353,12 @@ func Merge(outerCtx context.Context, prID int64, doer *user_model.User, mergeSty }, }) if err != nil { + // clear the intent only if the commit verifiably didn't reach the base branch + if merged, checkErr := hasPullRequestCommitBeenMerged(ctx, pr); checkErr == nil && !merged && pr.MergedCommitID != "" { + if clearErr := clearMergeIntent(ctx, pr); clearErr != nil { + log.Error("Unable to clear merge intent of %-v: %v", pr, clearErr) + } + } return err } } diff --git a/services/pull/merge_test.go b/services/pull/merge_test.go index 48c042c64e3..bb489e43c59 100644 --- a/services/pull/merge_test.go +++ b/services/pull/merge_test.go @@ -4,8 +4,12 @@ package pull import ( + "strings" "testing" + issues_model "gitea.dev/models/issues" + "gitea.dev/models/unittest" + user_model "gitea.dev/models/user" "gitea.dev/modules/git" "github.com/stretchr/testify/assert" @@ -137,3 +141,19 @@ func TestResolveMergeMessageTemplate(t *testing.T) { assert.Equal(t, "rebase template", tmpl) }) } + +func TestMergeIntentWithMissingCommit(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + ctx := t.Context() + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2}) + require.NoError(t, recordMergeIntent(ctx, pr, &user_model.User{ID: 2}, strings.Repeat("a", 40), false)) + + merged, err := hasPullRequestCommitBeenMerged(ctx, pr) + require.NoError(t, err) + assert.False(t, merged) + + require.NoError(t, clearMergeIntent(ctx, pr)) + pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2}) + assert.Empty(t, pr.MergedCommitID) + assert.Zero(t, pr.MergedUnix) +}