operations: Fix Go UserMergeBranch failing with ambiguous references

The UserMergeBranch RPC gets as input the branch name that is to be
merged into. This branch name is not fully qualified, so instead of
passing in "refs/heads/branch" the user only passes in "branch". We thus
need to make sure to convert it into a fully qualified reference, lest
any ambiguous reference may cause us to misbehave.

This causes us to mibehave with both Ruby and Go implementations of
UserMergeBranch. In Ruby, we silently compute the merge with the tag
instead of with the branch as the first parent, which is kind of the
worst thing we can do as it silently corrupts data. In Go, we at least
raise an error, which is arguably better behaviour compared to silently
computing the wrong merge.

This commit introduces a test which would trigger faulty behaviour in
both implementations by creating such an ambiguous reference and fixes
the bug in the Go implementation. The Ruby fix is likely more involved,
which may not be worthwhile to fix considering it's currently being
phased out anyway and has been a long standing bug. As such, the bug is
simply left as-is.

2 files changed, 84 insertions, 2 deletions

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)
 }