fix: merge autodetect can't close other PRs but only the last one when multiple PRs are pushed at once (#37512) (#37516)

Backport #37512

Fixes #37510.

Co-authored-by: Jason Learst <jason@jasonlearst.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

Author: 4 people

modules/git/catfile_batch_command.go

@@ -7,8 +7,10 @@ import (
 	"context"
 	"os"
 	"path/filepath"
+	"strings"
 
 	"code.gitea.io/gitea/modules/git/gitcmd"
+	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/util"
 )
 
@@ -39,6 +41,9 @@ func (b *catFileBatchCommand) getBatch() *catFileBatchCommunicator {
 }
 
 func (b *catFileBatchCommand) QueryContent(obj string) (*CatFileObject, BufferedReader, error) {
+	if strings.Contains(obj, "\n") {
+		setting.PanicInDevOrTesting("invalid object name with newline: %q", obj)
+	}
 	_, err := b.getBatch().reqWriter.Write([]byte("contents " + obj + "\n"))
 	if err != nil {
 		return nil, nil, err
@@ -51,6 +56,9 @@ func (b *catFileBatchCommand) QueryContent(obj string) (*CatFileObject, Buffered
 }
 
 func (b *catFileBatchCommand) QueryInfo(obj string) (*CatFileObject, error) {
+	if strings.Contains(obj, "\n") {
+		setting.PanicInDevOrTesting("invalid object name with newline: %q", obj)
+	}
 	_, err := b.getBatch().reqWriter.Write([]byte("info " + obj + "\n"))
 	if err != nil {
 		return nil, err

modules/git/catfile_batch_legacy.go

@@ -8,8 +8,10 @@ import (
 	"io"
 	"os"
 	"path/filepath"
+	"strings"
 
 	"code.gitea.io/gitea/modules/git/gitcmd"
+	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/util"
 )
 
@@ -50,6 +52,9 @@ func (b *catFileBatchLegacy) getBatchCheck() *catFileBatchCommunicator {
 }
 
 func (b *catFileBatchLegacy) QueryContent(obj string) (*CatFileObject, BufferedReader, error) {
+	if strings.Contains(obj, "\n") {
+		setting.PanicInDevOrTesting("invalid object name with newline: %q", obj)
+	}
 	_, err := io.WriteString(b.getBatchContent().reqWriter, obj+"\n")
 	if err != nil {
 		return nil, nil, err
@@ -62,6 +67,9 @@ func (b *catFileBatchLegacy) QueryContent(obj string) (*CatFileObject, BufferedR
 }
 
 func (b *catFileBatchLegacy) QueryInfo(obj string) (*CatFileObject, error) {
+	if strings.Contains(obj, "\n") {
+		setting.PanicInDevOrTesting("invalid object name with newline: %q", obj)
+	}
 	_, err := io.WriteString(b.getBatchCheck().reqWriter, obj+"\n")
 	if err != nil {
 		return nil, err

services/pull/check.go

@@ -313,18 +313,25 @@ func getMergeCommit(ctx context.Context, pr *issues_model.PullRequest) (*git.Com
 
 	objectFormat := git.ObjectFormatFromName(pr.BaseRepo.ObjectFormatName)
 
-	// Get the commit from BaseBranch where the pull request got merged
+	// Get the commit from BaseBranch where the pull request got merged.
+	// When several PRs targeting the same base are merged in a single push,
+	// rev-list returns one line per merge commit on the ancestry path; we
+	// only want the first one (the oldest, with --reverse, i.e. the merge
+	// commit that actually introduced this PR).
 	mergeCommit, _, err := gitrepo.RunCmdString(ctx, pr.BaseRepo,
 		gitcmd.NewCommand("rev-list", "--ancestry-path", "--merges", "--reverse").
 			AddDynamicArguments(prHeadCommitID+".."+pr.BaseBranch))
 	if err != nil {
 		return nil, fmt.Errorf("git rev-list --ancestry-path --merges --reverse: %w", err)
-	} else if len(mergeCommit) < objectFormat.FullLength() {
+	}
+
+	// only use the latest commit as merge commit if the output contains multiple commits
+	mergeCommit = strings.TrimSpace(mergeCommit)
+	mergeCommit, _, _ = strings.Cut(mergeCommit, "\n")
+	if len(mergeCommit) < objectFormat.FullLength() {
 		// PR was maybe fast-forwarded, so just use last commit of PR
 		mergeCommit = prHeadCommitID
 	}
-	mergeCommit = strings.TrimSpace(mergeCommit)
-
 	commit, err := gitRepo.GetCommit(mergeCommit)
 	if err != nil {
 		return nil, fmt.Errorf("GetMergeCommit[%s]: %w", mergeCommit, err)

tests/integration/manual_merge_test.go

@@ -6,13 +6,16 @@ package integration
 import (
 	"fmt"
 	"net/url"
+	"strings"
 	"testing"
 	"time"
 
 	auth_model "code.gitea.io/gitea/models/auth"
 	issues_model "code.gitea.io/gitea/models/issues"
+	repo_model "code.gitea.io/gitea/models/repo"
 	"code.gitea.io/gitea/models/unittest"
 	user_model "code.gitea.io/gitea/models/user"
+	"code.gitea.io/gitea/modules/git/gitcmd"
 	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
 
@@ -26,7 +29,6 @@ func TestManualMergeAutodetect(t *testing.T) {
 		// user1 is the pusher/merger
 		user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
 		user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
-		session2 := loginUser(t, user2.Name)
 
 		// Create a repo owned by user2
 		repoName := "manual-merge-autodetect"
@@ -41,35 +43,62 @@ func TestManualMergeAutodetect(t *testing.T) {
 			AutodetectManualMerge: new(true),
 		})(t)
 
-		// Create a PR from a branch
-		branchName := "feature"
-		testEditFileToNewBranch(t, session2, user2.Name, repoName, defaultBranch, branchName, "README.md", "Manual Merge Test")
+		repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{Name: repoName})
+		user1Ctx := NewAPITestContext(t, user1.Name, repoName, auth_model.AccessTokenScopeWriteRepository)
 
-		apiPull, err := doAPICreatePullRequest(NewAPITestContext(t, user1.Name, repoName, auth_model.AccessTokenScopeWriteRepository), user2.Name, repoName, defaultBranch, branchName)(t)
-		assert.NoError(t, err)
+		// multiple PRs should be able to be closed together if a push contains their branch commits.
+		branchNames := []string{"fix-1", "fix-2"}
+		apiPulls := make([]api.PullRequest, len(branchNames))
+		for i, branchName := range branchNames {
+			_, err := createFileInBranch(user2, repo,
+				createFileInBranchOptions{OldBranch: defaultBranch, NewBranch: branchName},
+				map[string]string{"test-file-" + branchName: "dummy"},
+			)
+			require.NoError(t, err)
+			apiPulls[i], err = doAPICreatePullRequest(user1Ctx, user2.Name, repoName, defaultBranch, branchName)(t)
+			require.NoError(t, err)
+		}
 
-		// user1 clones and pushes the branch to master (fast-forward)
+		// 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 ...".
 		dstPath := t.TempDir()
 		u, _ := url.Parse(giteaURL.String())
 		u.Path = fmt.Sprintf("%s/%s.git", user2.Name, repoName)
 		u.User = url.UserPassword(user1.Name, userPassword)
 
 		doGitClone(dstPath, u)(t)
-		doGitMerge(dstPath, "origin/"+branchName)(t)
+
+		// Capture each branch's expected merge commit hash from the local clone,
+		// so we can assert that Gitea recorded the correct merge commit per PR
+		// (and not just "some merge commit" — see the regression where every PR
+		// was attributed to the last merge in the push).
+		expectedMergeCommits := make([]string, len(branchNames))
+		for i, branchName := range branchNames {
+			doGitMerge(dstPath, "--no-ff", "-m", "merge "+branchName, "origin/"+branchName)(t)
+			head, _, cmdErr := gitcmd.NewCommand("rev-parse", "HEAD").WithDir(dstPath).RunStdString(t.Context())
+			require.NoError(t, cmdErr)
+			expectedMergeCommits[i] = strings.TrimSpace(head)
+		}
 		doGitPushTestRepository(dstPath, "origin", defaultBranch)(t)
 
-		// Wait for the PR to be marked as merged by the background task
-		var pr *issues_model.PullRequest
+		// Every PR should be detected as manually merged by the background task, not just the last one.
 		require.Eventually(t, func() bool {
-			pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: apiPull.ID})
-			return pr.HasMerged
+			for _, apiPull := range apiPulls {
+				pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: apiPull.ID})
+				if !pr.HasMerged {
+					return false
+				}
+			}
+			return true
 		}, 10*time.Second, 100*time.Millisecond)
 
-		// Check if the PR is merged and who is the merger
-		pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: apiPull.ID})
-		assert.True(t, pr.HasMerged)
-		assert.Equal(t, issues_model.PullRequestStatusManuallyMerged, pr.Status)
-		// Merger should be user1 (the pusher), not the commit author (user2) or repo owner (user2)
-		assert.Equal(t, user1.ID, pr.MergerID)
+		for i, apiPull := range apiPulls {
+			pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: apiPull.ID})
+			assert.Truef(t, pr.HasMerged, "PR %d (%s) should be marked as merged", i+1, branchNames[i])
+			assert.Equalf(t, issues_model.PullRequestStatusManuallyMerged, pr.Status, "PR %d (%s) should have ManuallyMerged status", i+1, branchNames[i])
+			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])
+		}
 	})
 }