Skip to content

Commit 5004bb0

Browse files
wxiaoguangwxiaoguang
committed
fix
1 parent eedfbda commit 5004bb0

2 files changed

Lines changed: 23 additions & 100 deletions

File tree

‎services/pull/check.go‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -350,12 +350,14 @@ func getMergeCommit(ctx context.Context, pr *issues_model.PullRequest) (*git.Com
350350
if err != nil {
351351
return nil, fmt.Errorf("git rev-list --ancestry-path --merges --reverse: %w", err)
352352
}
353-
mergeCommit, _, _ = strings.Cut(strings.TrimSpace(mergeCommit), "\n")
353+
354+
// only use the latest commit as merge commit if the output contains multiple commits
355+
mergeCommit = strings.TrimSpace(mergeCommit)
356+
mergeCommit, _, _ = strings.Cut(mergeCommit, "\n")
354357
if len(mergeCommit) < objectFormat.FullLength() {
355358
// PR was maybe fast-forwarded, so just use last commit of PR
356359
mergeCommit = prHeadCommitID
357360
}
358-
359361
commit, err := gitRepo.GetCommit(mergeCommit)
360362
if err != nil {
361363
return nil, fmt.Errorf("GetMergeCommit[%s]: %w", mergeCommit, err)

‎tests/integration/manual_merge_test.go‎

Lines changed: 19 additions & 98 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212

1313
auth_model "code.gitea.io/gitea/models/auth"
1414
issues_model "code.gitea.io/gitea/models/issues"
15+
repo_model "code.gitea.io/gitea/models/repo"
1516
"code.gitea.io/gitea/models/unittest"
1617
user_model "code.gitea.io/gitea/models/user"
1718
"code.gitea.io/gitea/modules/git/gitcmd"
@@ -28,9 +29,7 @@ func TestManualMergeAutodetect(t *testing.T) {
2829
// user1 is the pusher/merger
2930
user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
3031
user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
31-
session2 := loginUser(t, user2.Name)
3232

33-
// Create a repo owned by user2
3433
repoName := "manual-merge-autodetect"
3534
defaultBranch := setting.Repository.DefaultBranch
3635
user2Ctx := NewAPITestContext(t, user2.Name, repoName, auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser)
@@ -43,104 +42,31 @@ func TestManualMergeAutodetect(t *testing.T) {
4342
AutodetectManualMerge: new(true),
4443
})(t)
4544

46-
// Create a PR from a branch
47-
branchName := "feature"
48-
testEditFileToNewBranch(t, session2, user2.Name, repoName, defaultBranch, branchName, "README.md", "Manual Merge Test")
45+
repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{Name: repoName})
46+
user1Ctx := NewAPITestContext(t, user1.Name, repoName, auth_model.AccessTokenScopeWriteRepository)
4947

50-
apiPull, err := doAPICreatePullRequest(NewAPITestContext(t, user1.Name, repoName, auth_model.AccessTokenScopeWriteRepository), user2.Name, repoName, defaultBranch, branchName)(t)
51-
assert.NoError(t, err)
52-
53-
// user1 clones and pushes the branch to master (fast-forward)
54-
dstPath := t.TempDir()
55-
u, _ := url.Parse(giteaURL.String())
56-
u.Path = fmt.Sprintf("%s/%s.git", user2.Name, repoName)
57-
u.User = url.UserPassword(user1.Name, userPassword)
58-
59-
doGitClone(dstPath, u)(t)
60-
doGitMerge(dstPath, "origin/"+branchName)(t)
61-
doGitPushTestRepository(dstPath, "origin", defaultBranch)(t)
62-
63-
// Wait for the PR to be marked as merged by the background task
64-
var pr *issues_model.PullRequest
65-
require.Eventually(t, func() bool {
66-
pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: apiPull.ID})
67-
return pr.HasMerged
68-
}, 10*time.Second, 100*time.Millisecond)
69-
70-
// Check if the PR is merged and who is the merger
71-
pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: apiPull.ID})
72-
assert.True(t, pr.HasMerged)
73-
assert.Equal(t, issues_model.PullRequestStatusManuallyMerged, pr.Status)
74-
// Merger should be user1 (the pusher), not the commit author (user2) or repo owner (user2)
75-
assert.Equal(t, user1.ID, pr.MergerID)
76-
})
77-
}
78-
79-
// TestManualMergeAutodetectMultiplePRs covers the case where several PRs targeting
80-
// the same base branch are merged locally in sequence and pushed in a single push.
81-
//
82-
// Reproduces the regression where only the last PR's merge commit is detected.
83-
// The "git rev-list --ancestry-path --merges --reverse <prHead>..<base>" output
84-
// in services/pull/check.go:getMergeCommit returns multiple lines for any PR that
85-
// is not the most recent merge, and the multi-line value is forwarded to
86-
// gitRepo.GetCommit, which (since Gitea moved to "git cat-file --batch-command"
87-
// in #35775) kills the repository's cached cat-file process and breaks the
88-
// follow-up commit lookup.
89-
func TestManualMergeAutodetectMultiplePRs(t *testing.T) {
90-
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
91-
// user2 is the repo owner
92-
// user1 is the pusher/merger
93-
user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
94-
user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
95-
session2 := loginUser(t, user2.Name)
96-
97-
// Create a repo owned by user2
98-
repoName := "manual-merge-autodetect-multi"
99-
defaultBranch := setting.Repository.DefaultBranch
100-
user2Ctx := NewAPITestContext(t, user2.Name, repoName, auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser)
101-
doAPICreateRepository(user2Ctx, false)(t)
102-
103-
// Enable autodetect manual merge
104-
doAPIEditRepository(user2Ctx, &api.EditRepoOption{
105-
HasPullRequests: new(true),
106-
AllowManualMerge: new(true),
107-
AutodetectManualMerge: new(true),
108-
})(t)
109-
110-
// Create three branches from the default branch, each adding its own file,
111-
// and open a PR for each of them targeting the default branch. Each branch
112-
// touches a distinct file so the sequential merges don't conflict.
113-
branchNames := []string{"fix-1", "fix-2", "fix-3"}
48+
// multiple PRs should be able to be closed together if a commit contains their branch commits.
49+
branchNames := []string{"fix-1", "fix-2"}
11450
apiPulls := make([]api.PullRequest, len(branchNames))
11551
for i, branchName := range branchNames {
116-
testCreateFile(t, session2, user2.Name, repoName, defaultBranch, branchName,
117-
fmt.Sprintf("file-%d.txt", i+1), fmt.Sprintf("manual merge multi-PR test %d", i+1))
118-
119-
pr, err := doAPICreatePullRequest(
120-
NewAPITestContext(t, user1.Name, repoName, auth_model.AccessTokenScopeWriteRepository),
121-
user2.Name, repoName, defaultBranch, branchName,
122-
)(t)
52+
_, err := createFileInBranch(user2, repo,
53+
createFileInBranchOptions{OldBranch: defaultBranch, NewBranch: branchName},
54+
map[string]string{"test-file-" + branchName: "dummy"},
55+
)
56+
require.NoError(t, err)
57+
apiPulls[i], err = doAPICreatePullRequest(user1Ctx, user2.Name, repoName, defaultBranch, branchName)(t)
12358
require.NoError(t, err)
124-
apiPulls[i] = pr
12559
}
12660

12761
// user1 clones, then merges every branch sequentially, then pushes once.
128-
// The first merge fast-forwards; the rest produce real merge commits, which
129-
// is exactly the situation that breaks "ancestry-path --merges --reverse".
62+
// The first merge fast-forwards; the rest produce real merge commits,
63+
// which generates multiple commits for "git ancestry-path --merges --reverse".
13064
dstPath := t.TempDir()
13165
u, _ := url.Parse(giteaURL.String())
13266
u.Path = fmt.Sprintf("%s/%s.git", user2.Name, repoName)
13367
u.User = url.UserPassword(user1.Name, userPassword)
13468

13569
doGitClone(dstPath, u)(t)
136-
// Set a committer/author on the local clone so "git merge" can create merge
137-
// commits without falling back to a global identity.
138-
_, _, err := gitcmd.NewCommand("config", "user.email").AddDynamicArguments(user1.Email).
139-
WithDir(dstPath).RunStdString(t.Context())
140-
require.NoError(t, err)
141-
_, _, err = gitcmd.NewCommand("config", "user.name").AddDynamicArguments(user1.Name).
142-
WithDir(dstPath).RunStdString(t.Context())
143-
require.NoError(t, err)
14470

14571
// Capture each branch's expected merge commit hash from the local clone,
14672
// so we can assert that Gitea recorded the correct merge commit per PR
@@ -149,15 +75,13 @@ func TestManualMergeAutodetectMultiplePRs(t *testing.T) {
14975
expectedMergeCommits := make([]string, len(branchNames))
15076
for i, branchName := range branchNames {
15177
doGitMerge(dstPath, "--no-ff", "-m", "merge "+branchName, "origin/"+branchName)(t)
152-
head, _, cmdErr := gitcmd.NewCommand("rev-parse", "HEAD").
153-
WithDir(dstPath).RunStdString(t.Context())
78+
head, _, cmdErr := gitcmd.NewCommand("rev-parse", "HEAD").WithDir(dstPath).RunStdString(t.Context())
15479
require.NoError(t, cmdErr)
15580
expectedMergeCommits[i] = strings.TrimSpace(head)
15681
}
15782
doGitPushTestRepository(dstPath, "origin", defaultBranch)(t)
15883

159-
// Every PR should be detected as manually merged by the background task,
160-
// not just the last one.
84+
// Every PR should be detected as manually merged by the background task, not just the last one.
16185
require.Eventually(t, func() bool {
16286
for _, apiPull := range apiPulls {
16387
pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: apiPull.ID})
@@ -166,17 +90,14 @@ func TestManualMergeAutodetectMultiplePRs(t *testing.T) {
16690
}
16791
}
16892
return true
169-
}, 15*time.Second, 200*time.Millisecond)
93+
}, 10*time.Second, 100*time.Millisecond)
17094

17195
for i, apiPull := range apiPulls {
17296
pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: apiPull.ID})
17397
assert.Truef(t, pr.HasMerged, "PR %d (%s) should be marked as merged", i+1, branchNames[i])
174-
assert.Equalf(t, issues_model.PullRequestStatusManuallyMerged, pr.Status,
175-
"PR %d (%s) should have ManuallyMerged status", i+1, branchNames[i])
176-
assert.Equalf(t, user1.ID, pr.MergerID,
177-
"PR %d (%s) merger should be the pusher", i+1, branchNames[i])
178-
assert.Equalf(t, expectedMergeCommits[i], pr.MergedCommitID,
179-
"PR %d (%s) should be attributed to its own merge commit, not another PR's", i+1, branchNames[i])
98+
assert.Equalf(t, issues_model.PullRequestStatusManuallyMerged, pr.Status, "PR %d (%s) should have ManuallyMerged status", i+1, branchNames[i])
99+
assert.Equalf(t, user1.ID, pr.MergerID, "PR %d (%s) merger should be the pusher", i+1, branchNames[i])
100+
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])
180101
}
181102
})
182103
}

0 commit comments

Comments
 (0)