mirror of
https://github.com/go-gitea/gitea.git
synced 2026-03-17 14:24:07 +00:00
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>
This commit is contained in:
@@ -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,18 +384,11 @@ 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)
|
||||
merger, err := getMergerForManuallyMergedPullRequest(ctx, pr)
|
||||
if err != nil {
|
||||
log.Error("%-v getMergerForManuallyMergedPullRequest: %v", pr, err)
|
||||
return false
|
||||
}
|
||||
}
|
||||
merger = pr.BaseRepo.Owner
|
||||
}
|
||||
|
||||
if merged, err := SetMerged(ctx, pr, commit.ID.String(), timeutil.TimeStamp(commit.Author.When.Unix()), merger, issues_model.PullRequestStatusManuallyMerged); err != nil {
|
||||
log.Error("%-v setMerged : %v", pr, err)
|
||||
|
||||
75
tests/integration/manual_merge_test.go
Normal file
75
tests/integration/manual_merge_test.go
Normal file
@@ -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)
|
||||
})
|
||||
}
|
||||
Reference in New Issue
Block a user