diff options
| -rw-r--r-- | changelogs/unreleased/pks-user-merge-branch-ambiguous-ref.yml | 5 | ||||
| -rw-r--r-- | internal/gitaly/service/operations/merge.go | 4 | ||||
| -rw-r--r-- | internal/gitaly/service/operations/merge_test.go | 82 |
3 files changed, 89 insertions, 2 deletions
diff --git a/changelogs/unreleased/pks-user-merge-branch-ambiguous-ref.yml b/changelogs/unreleased/pks-user-merge-branch-ambiguous-ref.yml new file mode 100644 index 000000000..99258230a --- /dev/null +++ b/changelogs/unreleased/pks-user-merge-branch-ambiguous-ref.yml @@ -0,0 +1,5 @@ +--- +title: 'operations: Fix Go UserMergeBranch failing with ambiguous references' +merge_request: 2921 +author: +type: fixed diff --git a/internal/gitaly/service/operations/merge.go b/internal/gitaly/service/operations/merge.go index f4c729bb8..6dd16f82a 100644 --- a/internal/gitaly/service/operations/merge.go +++ b/internal/gitaly/service/operations/merge.go @@ -114,7 +114,8 @@ func (s *Server) userMergeBranch(stream gitalypb.OperationService_UserMergeBranc return err } - revision, err := git.NewRepository(repo).ResolveRefish(ctx, string(firstRequest.Branch)) + branch := "refs/heads/" + text.ChompBytes(firstRequest.Branch) + revision, err := git.NewRepository(repo).ResolveRefish(ctx, branch) if err != nil { return err } @@ -148,7 +149,6 @@ func (s *Server) userMergeBranch(stream gitalypb.OperationService_UserMergeBranc return helper.ErrPreconditionFailedf("merge aborted by client") } - branch := "refs/heads/" + text.ChompBytes(firstRequest.Branch) if err := s.updateReferenceWithHooks(ctx, firstRequest.Repository, firstRequest.User, branch, merge.CommitID, revision); err != nil { var preReceiveError preReceiveError var updateRefError updateRefError diff --git a/internal/gitaly/service/operations/merge_test.go b/internal/gitaly/service/operations/merge_test.go index 1f3ec0240..3f4e1c735 100644 --- a/internal/gitaly/service/operations/merge_test.go +++ b/internal/gitaly/service/operations/merge_test.go @@ -12,8 +12,10 @@ import ( "time" "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitaly/internal/git" gitlog "gitlab.com/gitlab-org/gitaly/internal/git/log" "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" + "gitlab.com/gitlab-org/gitaly/internal/helper" "gitlab.com/gitlab-org/gitaly/internal/helper/text" "gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/internal/testhelper" @@ -244,6 +246,86 @@ func testFailedMergeConcurrentUpdate(t *testing.T, ctx context.Context) { require.Equal(t, commit.Id, concurrentCommitID, "RPC should not have trampled concurrent update") } +func TestUserMergeBranch_ambiguousReference(t *testing.T) { + testWithFeature(t, featureflag.GoUserMergeBranch, testUserMergeBranchAmbiguousReference) +} + +func testUserMergeBranchAmbiguousReference(t *testing.T, ctx context.Context) { + testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t) + defer cleanupFn() + + serverSocketPath, stop := runOperationServiceServer(t) + defer stop() + + client, conn := newOperationClient(t, serverSocketPath) + defer conn.Close() + + merge, err := client.UserMergeBranch(ctx) + require.NoError(t, err) + + prepareMergeBranch(t, testRepoPath) + + repo := git.NewRepository(testRepo) + + master, err := repo.GetReference(ctx, "refs/heads/master") + require.NoError(t, err) + + // We're now creating all kinds of potentially ambiguous references in + // the hope that UserMergeBranch won't be confused by it. + for _, reference := range []string{ + mergeBranchName, + "heads/" + mergeBranchName, + "refs/heads/refs/heads/" + mergeBranchName, + "refs/tags/" + mergeBranchName, + "refs/tags/heads/" + mergeBranchName, + "refs/tags/refs/heads/" + mergeBranchName, + } { + require.NoError(t, repo.UpdateRef(ctx, reference, master.Target, git.NullSHA)) + } + + mergeCommitMessage := "Merged by Gitaly" + firstRequest := &gitalypb.UserMergeBranchRequest{ + Repository: testRepo, + User: testhelper.TestUser, + CommitId: commitToMerge, + Branch: []byte(mergeBranchName), + Message: []byte(mergeCommitMessage), + } + + require.NoError(t, merge.Send(firstRequest), "send first request") + + _, err = merge.Recv() + require.NoError(t, err, "receive first response") + require.NoError(t, err, "look up git commit before merge is applied") + require.NoError(t, merge.Send(&gitalypb.UserMergeBranchRequest{Apply: true}), "apply merge") + + response, err := merge.Recv() + require.NoError(t, err, "receive second response") + require.NoError(t, testhelper.ReceiveEOFWithTimeout(func() error { + _, err = merge.Recv() + return err + })) + + commit, err := gitlog.GetCommit(ctx, testRepo, "refs/heads/"+mergeBranchName) + require.NoError(t, err, "look up git commit after call has finished") + + tag, err := gitlog.GetCommit(ctx, testRepo, "refs/tags/"+mergeBranchName) + require.NoError(t, err, "look up git tag after call has finished") + + require.Equal(t, gitalypb.OperationBranchUpdate{CommitId: commit.Id}, *(response.BranchUpdate)) + require.Equal(t, mergeCommitMessage, string(commit.Body)) + require.Equal(t, testhelper.TestUser.Name, commit.Author.Name) + require.Equal(t, testhelper.TestUser.Email, commit.Author.Email) + + if featureflag.IsEnabled(helper.OutgoingToIncoming(ctx), featureflag.GoUserMergeBranch) { + require.Equal(t, []string{mergeBranchHeadBefore, commitToMerge}, commit.ParentIds) + } else { + // The Ruby implementation performs a mismerge with the + // ambiguous tag instead of with the branch name. + require.Equal(t, []string{tag.Id, commitToMerge}, commit.ParentIds) + } +} + func TestFailedMergeDueToHooks(t *testing.T) { testWithFeature(t, featureflag.GoUserMergeBranch, testFailedMergeDueToHooks) } |