Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions modules/git/catfile_batch_command.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
8 changes: 8 additions & 0 deletions modules/git/catfile_batch_legacy.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
15 changes: 11 additions & 4 deletions services/pull/check.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
65 changes: 47 additions & 18 deletions tests/integration/manual_merge_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand All @@ -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"
Expand All @@ -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])
}
})
}