fix(pull): don't auto-detect empty pull requests as manually merged (#39807) (#39812)

Backport #39807 by @gomitrah

With "Autodetect manual merge" enabled, an empty pull request (one with
no commits of its own, for example opened to discuss a branch) was
closed as manually merged as soon as it was checked. Its head commit is
already an ancestor of the base branch, so the detection took the
"fast-forwarded" path and recorded the head itself as the merge commit,
even though nothing was merged.

Detection now skips a PR whose head commit is its merge base, the same
condition Gitea already uses to show a PR as "already included in the
target branch". Real manual merges are still detected, including when
the base branch and the PR branch are pushed together.

The merge base is refreshed on recheck, so a fast-forward manual merge
whose merge base was already refreshed to the head is not detected
either. This only misses a detection and never closes a PR wrongly;
telling the two apart would need a value stored at PR creation.

Fixes https://github.com/go-gitea/gitea/issues/31947

Co-authored-by: Mitrahsoft <bala.c@mitrahsoft.in>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

services/pull/check.go

@@ -29,6 +29,7 @@ import (
 	"gitea.dev/modules/queue"
 	"gitea.dev/modules/setting"
 	"gitea.dev/modules/timeutil"
+	"gitea.dev/modules/util"
 	asymkey_service "gitea.dev/services/asymkey"
 	"gitea.dev/services/automergequeue"
 )
@@ -313,8 +314,7 @@ func getMergeCommit(ctx context.Context, pr *issues_model.PullRequest) (*git.Com
 	cmd := gitcmd.NewCommand("merge-base", "--is-ancestor").AddDynamicArguments(prHeadRef, pr.BaseBranch)
 	if err := cmd.WithRepo(pr.BaseRepo).RunWithStderr(ctx); err != nil {
 		if gitcmd.IsErrorExitCode(err, 1) {
-			// prHeadRef is not an ancestor of the base branch
-			return nil, nil //nolint:nilnil // return nil to indicate that the PR head is not merged
+			return nil, util.ErrNotExist // prHeadRef is not an ancestor of the base branch, no merge commit is found
 		}
 		// Errors are signaled by a non-zero status that is not 1
 		return nil, fmt.Errorf("%-v git merge-base --is-ancestor: %w", pr, err)
@@ -327,6 +327,9 @@ func getMergeCommit(ctx context.Context, pr *issues_model.PullRequest) (*git.Com
 	if err != nil {
 		return nil, fmt.Errorf("GetFullCommitID(%s) in %s: %w", prHeadRef, pr.BaseRepo.FullName(), err)
 	}
+	if prHeadCommitID == pr.MergeBase {
+		return nil, util.ErrNotExist // empty PR, no merge commit is found
+	}
 
 	gitRepo, err := git.OpenRepository(ctx, pr.BaseRepo)
 	if err != nil {
@@ -403,13 +406,10 @@ func manuallyMergedByPushedCommit(ctx context.Context, pr *issues_model.PullRequ
 	}
 
 	commit, err := getMergeCommit(ctx, pr)
-	if err != nil {
-		log.Error("%-v getMergeCommit: %v", pr, err)
+	if errors.Is(err, util.ErrNotExist) {
 		return false
-	}
-
-	if commit == nil {
-		// no merge commit found
+	} else if err != nil {
+		log.Error("%-v getMergeCommit: %v", pr, err)
 		return false
 	}
 

tests/integration/manual_merge_test.go

@@ -5,6 +5,7 @@ package integration
 
 import (
 	"fmt"
+	"net/http"
 	"net/url"
 	"strings"
 	"testing"
@@ -16,6 +17,7 @@ import (
 	"gitea.dev/models/unittest"
 	user_model "gitea.dev/models/user"
 	"gitea.dev/modules/git/gitcmd"
+	"gitea.dev/modules/queue"
 	"gitea.dev/modules/setting"
 	api "gitea.dev/modules/structs"
 
@@ -59,6 +61,10 @@ func TestManualMergeAutodetect(t *testing.T) {
 			require.NoError(t, err)
 		}
 
+		testAPICreateBranch(t, user2Ctx.Session, user2.Name, repoName, defaultBranch, "empty-pr", http.StatusCreated)
+		emptyAPIPull, err := doAPICreatePullRequest(user1Ctx, user2.Name, repoName, defaultBranch, "empty-pr")(t)
+		require.NoError(t, err)
+
 		// user1 clones, then merges every branch sequentially, then pushes once.
 		// The first merge fast-forwards; the rest produce real merge commits,
 		// which generates multiple commits for "git rev-list --ancestry-path --merges ...".
@@ -100,5 +106,10 @@ func TestManualMergeAutodetect(t *testing.T) {
 			assert.Equalf(t, user1.ID, pr.MergerID, "PR %d (%s) merger should be the pusher", i+1, branchNames[i])
 			assert.Equalf(t, expectedMergeCommits[i], pr.MergedCommitID, "PR %d (%s) should be attributed to its own merge commit, not another PR's", i+1, branchNames[i])
 		}
+
+		require.NoError(t, queue.GetManager().FlushAll(t.Context(), 5*time.Second))
+		emptyPR := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: emptyAPIPull.ID})
+		assert.False(t, emptyPR.HasMerged, "empty PR must not be detected as manually merged")
+		assert.Equal(t, issues_model.PullRequestStatusAncestor, emptyPR.Status)
 	})
 }