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 <xiaolunwen@gmail.com>
Co-authored-by: silverwind <me@silverwind.io>

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

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) {

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
 			}
 		}

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)
+}