diff --git a/modules/git/catfile_batch_command.go b/modules/git/catfile_batch_command.go index 710561f045..4e18282bf3 100644 --- a/modules/git/catfile_batch_command.go +++ b/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 diff --git a/modules/git/catfile_batch_legacy.go b/modules/git/catfile_batch_legacy.go index 795fc4ce3d..595043d1d2 100644 --- a/modules/git/catfile_batch_legacy.go +++ b/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 diff --git a/services/pull/check.go b/services/pull/check.go index bc706844b2..241eb633b6 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -339,18 +339,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) diff --git a/tests/integration/manual_merge_test.go b/tests/integration/manual_merge_test.go index 4cfba9a225..7f931b620b 100644 --- a/tests/integration/manual_merge_test.go +++ b/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]) + } }) }