Skip to content

Commit 37f6f7f

Browse files
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 <wxiaoguang@gmail.com>
1 parent 0e0daa8 commit 37f6f7f

2 files changed

Lines changed: 101 additions & 11 deletions

File tree

services/pull/check.go

Lines changed: 26 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -333,6 +333,28 @@ func getMergeCommit(ctx context.Context, pr *issues_model.PullRequest) (*git.Com
333333
return commit, nil
334334
}
335335

336+
func getMergerForManuallyMergedPullRequest(ctx context.Context, pr *issues_model.PullRequest) (*user_model.User, error) {
337+
var errs []error
338+
if branch, err := git_model.GetBranch(ctx, pr.BaseRepoID, pr.BaseBranch); err != nil {
339+
errs = append(errs, err)
340+
} else {
341+
err := branch.LoadPusher(ctx) // LoadPusher uses ghost for non-existing user
342+
if branch.Pusher != nil && branch.Pusher.ID > 0 {
343+
return branch.Pusher, nil
344+
} else if err != nil {
345+
errs = append(errs, err)
346+
}
347+
}
348+
349+
// When the doer (pusher) is unknown set the BaseRepo owner as merger
350+
err := pr.BaseRepo.LoadOwner(ctx)
351+
if err == nil {
352+
return pr.BaseRepo.Owner, nil
353+
}
354+
errs = append(errs, err)
355+
return nil, fmt.Errorf("unable to find merger for manually merged pull request: %w", errors.Join(errs...))
356+
}
357+
336358
// manuallyMerged checks if a pull request got manually merged
337359
// When a pull request got manually merged mark the pull request as merged
338360
func manuallyMerged(ctx context.Context, pr *issues_model.PullRequest) bool {
@@ -362,17 +384,10 @@ func manuallyMerged(ctx context.Context, pr *issues_model.PullRequest) bool {
362384
return false
363385
}
364386

365-
merger, _ := user_model.GetUserByEmail(ctx, commit.Author.Email)
366-
367-
// When the commit author is unknown set the BaseRepo owner as merger
368-
if merger == nil {
369-
if pr.BaseRepo.Owner == nil {
370-
if err = pr.BaseRepo.LoadOwner(ctx); err != nil {
371-
log.Error("%-v BaseRepo.LoadOwner: %v", pr, err)
372-
return false
373-
}
374-
}
375-
merger = pr.BaseRepo.Owner
387+
merger, err := getMergerForManuallyMergedPullRequest(ctx, pr)
388+
if err != nil {
389+
log.Error("%-v getMergerForManuallyMergedPullRequest: %v", pr, err)
390+
return false
376391
}
377392

378393
if merged, err := SetMerged(ctx, pr, commit.ID.String(), timeutil.TimeStamp(commit.Author.When.Unix()), merger, issues_model.PullRequestStatusManuallyMerged); err != nil {
Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
// Copyright 2026 The Gitea Authors. All rights reserved.
2+
// SPDX-License-Identifier: MIT
3+
4+
package integration
5+
6+
import (
7+
"fmt"
8+
"net/url"
9+
"testing"
10+
"time"
11+
12+
auth_model "code.gitea.io/gitea/models/auth"
13+
issues_model "code.gitea.io/gitea/models/issues"
14+
"code.gitea.io/gitea/models/unittest"
15+
user_model "code.gitea.io/gitea/models/user"
16+
"code.gitea.io/gitea/modules/setting"
17+
api "code.gitea.io/gitea/modules/structs"
18+
19+
"github.com/stretchr/testify/assert"
20+
"github.com/stretchr/testify/require"
21+
)
22+
23+
func TestManualMergeAutodetect(t *testing.T) {
24+
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
25+
// user2 is the repo owner
26+
// user1 is the pusher/merger
27+
user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
28+
user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
29+
session2 := loginUser(t, user2.Name)
30+
31+
// Create a repo owned by user2
32+
repoName := "manual-merge-autodetect"
33+
defaultBranch := setting.Repository.DefaultBranch
34+
user2Ctx := NewAPITestContext(t, user2.Name, repoName, auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser)
35+
doAPICreateRepository(user2Ctx, false)(t)
36+
37+
// Enable autodetect manual merge
38+
doAPIEditRepository(user2Ctx, &api.EditRepoOption{
39+
HasPullRequests: new(true),
40+
AllowManualMerge: new(true),
41+
AutodetectManualMerge: new(true),
42+
})(t)
43+
44+
// Create a PR from a branch
45+
branchName := "feature"
46+
testEditFileToNewBranch(t, session2, user2.Name, repoName, defaultBranch, branchName, "README.md", "Manual Merge Test")
47+
48+
apiPull, err := doAPICreatePullRequest(NewAPITestContext(t, user1.Name, repoName, auth_model.AccessTokenScopeWriteRepository), user2.Name, repoName, defaultBranch, branchName)(t)
49+
assert.NoError(t, err)
50+
51+
// user1 clones and pushes the branch to master (fast-forward)
52+
dstPath := t.TempDir()
53+
u, _ := url.Parse(giteaURL.String())
54+
u.Path = fmt.Sprintf("%s/%s.git", user2.Name, repoName)
55+
u.User = url.UserPassword(user1.Name, userPassword)
56+
57+
doGitClone(dstPath, u)(t)
58+
doGitMerge(dstPath, "origin/"+branchName)(t)
59+
doGitPushTestRepository(dstPath, "origin", defaultBranch)(t)
60+
61+
// Wait for the PR to be marked as merged by the background task
62+
var pr *issues_model.PullRequest
63+
require.Eventually(t, func() bool {
64+
pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: apiPull.ID})
65+
return pr.HasMerged
66+
}, 10*time.Second, 100*time.Millisecond)
67+
68+
// Check if the PR is merged and who is the merger
69+
pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: apiPull.ID})
70+
assert.True(t, pr.HasMerged)
71+
assert.Equal(t, issues_model.PullRequestStatusManuallyMerged, pr.Status)
72+
// Merger should be user1 (the pusher), not the commit author (user2) or repo owner (user2)
73+
assert.Equal(t, user1.ID, pr.MergerID)
74+
})
75+
}

0 commit comments

Comments
 (0)