From 37f6f7f6d4896f8c48839b97c298f1f309dd1354 Mon Sep 17 00:00:00 2001 From: Adam Majer Date: Mon, 2 Mar 2026 21:05:58 +0100 Subject: [PATCH] Pull Request Pusher should be the author of the merge (#36581) In manual merge detected changes, the pushing user should be the de-facto author of the merge, not the committer. For ff-only merges, the author (PR owner) often have nothing to do with the merger. Similarly, even if a merge commit exists, it does not indicate that the merge commit author is the merger. This is especially true if the merge commit is a ff-only merge on a given branch. If pusher is for some reason unavailable, we fall back to the old method of using committer or owning organization as the author. --------- Co-authored-by: wxiaoguang --- services/pull/check.go | 37 +++++++++---- tests/integration/manual_merge_test.go | 75 ++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 11 deletions(-) create mode 100644 tests/integration/manual_merge_test.go diff --git a/services/pull/check.go b/services/pull/check.go index f6e8433cf26..6486ca79df3 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -333,6 +333,28 @@ func getMergeCommit(ctx context.Context, pr *issues_model.PullRequest) (*git.Com return commit, nil } +func getMergerForManuallyMergedPullRequest(ctx context.Context, pr *issues_model.PullRequest) (*user_model.User, error) { + var errs []error + if branch, err := git_model.GetBranch(ctx, pr.BaseRepoID, pr.BaseBranch); err != nil { + errs = append(errs, err) + } else { + err := branch.LoadPusher(ctx) // LoadPusher uses ghost for non-existing user + if branch.Pusher != nil && branch.Pusher.ID > 0 { + return branch.Pusher, nil + } else if err != nil { + errs = append(errs, err) + } + } + + // When the doer (pusher) is unknown set the BaseRepo owner as merger + err := pr.BaseRepo.LoadOwner(ctx) + if err == nil { + return pr.BaseRepo.Owner, nil + } + errs = append(errs, err) + return nil, fmt.Errorf("unable to find merger for manually merged pull request: %w", errors.Join(errs...)) +} + // manuallyMerged checks if a pull request got manually merged // When a pull request got manually merged mark the pull request as merged func manuallyMerged(ctx context.Context, pr *issues_model.PullRequest) bool { @@ -362,17 +384,10 @@ func manuallyMerged(ctx context.Context, pr *issues_model.PullRequest) bool { return false } - merger, _ := user_model.GetUserByEmail(ctx, commit.Author.Email) - - // When the commit author is unknown set the BaseRepo owner as merger - if merger == nil { - if pr.BaseRepo.Owner == nil { - if err = pr.BaseRepo.LoadOwner(ctx); err != nil { - log.Error("%-v BaseRepo.LoadOwner: %v", pr, err) - return false - } - } - merger = pr.BaseRepo.Owner + merger, err := getMergerForManuallyMergedPullRequest(ctx, pr) + if err != nil { + log.Error("%-v getMergerForManuallyMergedPullRequest: %v", pr, err) + return false } if merged, err := SetMerged(ctx, pr, commit.ID.String(), timeutil.TimeStamp(commit.Author.When.Unix()), merger, issues_model.PullRequestStatusManuallyMerged); err != nil { diff --git a/tests/integration/manual_merge_test.go b/tests/integration/manual_merge_test.go new file mode 100644 index 00000000000..4cfba9a225e --- /dev/null +++ b/tests/integration/manual_merge_test.go @@ -0,0 +1,75 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package integration + +import ( + "fmt" + "net/url" + "testing" + "time" + + auth_model "code.gitea.io/gitea/models/auth" + issues_model "code.gitea.io/gitea/models/issues" + "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/setting" + api "code.gitea.io/gitea/modules/structs" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestManualMergeAutodetect(t *testing.T) { + onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { + // user2 is the repo owner + // 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" + defaultBranch := setting.Repository.DefaultBranch + user2Ctx := NewAPITestContext(t, user2.Name, repoName, auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser) + doAPICreateRepository(user2Ctx, false)(t) + + // Enable autodetect manual merge + doAPIEditRepository(user2Ctx, &api.EditRepoOption{ + HasPullRequests: new(true), + AllowManualMerge: new(true), + 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") + + apiPull, err := doAPICreatePullRequest(NewAPITestContext(t, user1.Name, repoName, auth_model.AccessTokenScopeWriteRepository), user2.Name, repoName, defaultBranch, branchName)(t) + assert.NoError(t, err) + + // user1 clones and pushes the branch to master (fast-forward) + 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) + doGitPushTestRepository(dstPath, "origin", defaultBranch)(t) + + // Wait for the PR to be marked as merged by the background task + var pr *issues_model.PullRequest + require.Eventually(t, func() bool { + pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: apiPull.ID}) + return pr.HasMerged + }, 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) + }) +}