Merge branch 'pks-user-ff-branch-ambiguous-ref' into 'master'

operations: Fix UserFFBranch if called on an ambiguous reference

See merge request gitlab-org/gitaly!2992

6 files changed, 110 insertions, 22 deletions

diff --git a/changelogs/unreleased/pks-user-ff-branch-ambiguous-ref.yml b/changelogs/unreleased/pks-user-ff-branch-ambiguous-ref.yml
new file mode 100644
index 000000000..2738ba664
--- /dev/null
+++ b/changelogs/unreleased/pks-user-ff-branch-ambiguous-ref.yml
@@ -0,0 +1,5 @@
+---
+title: 'operations: Fix UserFFBranch if called on an ambiguous reference'
+merge_request: 2992
+author:
+type: fixed
diff --git a/internal/gitaly/service/operations/merge.go b/internal/gitaly/service/operations/merge.go
index 25e8dc533..09ab7d1d6 100644
--- a/internal/gitaly/service/operations/merge.go
+++ b/internal/gitaly/service/operations/merge.go
@@ -198,7 +198,8 @@ func (s *Server) UserFFBranch(ctx context.Context, in *gitalypb.UserFFBranchRequ
 		return s.userFFBranchRuby(ctx, in)
 	}
 
-	revision, err := git.NewRepository(in.Repository).ResolveRefish(ctx, string(in.Branch))
+	branch := fmt.Sprintf("refs/heads/%s", in.Branch)
+	revision, err := git.NewRepository(in.Repository).ResolveRefish(ctx, branch)
 	if err != nil {
 		return nil, helper.ErrInvalidArgument(err)
 	}
@@ -211,7 +212,6 @@ func (s *Server) UserFFBranch(ctx context.Context, in *gitalypb.UserFFBranchRequ
 		return nil, helper.ErrPreconditionFailedf("not fast forward")
 	}
 
-	branch := fmt.Sprintf("refs/heads/%s", in.Branch)
 	if err := s.updateReferenceWithHooks(ctx, in.Repository, in.User, branch, in.CommitId, revision); err != nil {
 		var preReceiveError preReceiveError
 		if errors.As(err, &preReceiveError) {
diff --git a/internal/gitaly/service/operations/merge_test.go b/internal/gitaly/service/operations/merge_test.go
index 94ded8d8e..63b8b5a6c 100644
--- a/internal/gitaly/service/operations/merge_test.go
+++ b/internal/gitaly/service/operations/merge_test.go
@@ -424,7 +424,6 @@ func TestSuccessfulUserFFBranchRequest(t *testing.T) {
 		}
 
 		testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", "-f", branchName, "6d394385cf567f80a8fd85055db1ab4c5295806f")
-		defer exec.Command(config.Config.Git.BinPath, "-C", testRepoPath, "branch", "-d", branchName).Run()
 
 		resp, err := client.UserFFBranch(ctx, request)
 		require.NoError(t, err)
@@ -448,7 +447,6 @@ func TestFailedUserFFBranchRequest(t *testing.T) {
 	branchName := "test-ff-target-branch"
 
 	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", "-f", branchName, "6d394385cf567f80a8fd85055db1ab4c5295806f")
-	defer exec.Command(config.Config.Git.BinPath, "-C", testRepoPath, "branch", "-d", branchName).Run()
 
 	testCases := []struct {
 		desc     string
@@ -550,7 +548,6 @@ func TestFailedUserFFBranchDueToHooks(t *testing.T) {
 	}
 
 	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", "-f", branchName, "6d394385cf567f80a8fd85055db1ab4c5295806f")
-	defer exec.Command(config.Config.Git.BinPath, "-C", testRepoPath, "branch", "-d", branchName).Run()
 
 	hookContent := []byte("#!/bin/sh\necho 'failure'\nexit 1")
 
@@ -579,6 +576,58 @@ func TestFailedUserFFBranchDueToHooks(t *testing.T) {
 	}
 }
 
+func TestUserFFBranch_ambiguousReference(t *testing.T) {
+	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
+		featureflag.GoUserFFBranch,
+	}).Run(t, func(t *testing.T, ctx context.Context) {
+		serverSocketPath, stop := runOperationServiceServer(t)
+		defer stop()
+
+		client, conn := newOperationClient(t, serverSocketPath)
+		defer func() { require.NoError(t, conn.Close()) }()
+
+		testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
+		defer cleanupFn()
+
+		branchName := "test-ff-target-branch"
+
+		// We're creating both a branch and a tag with the same name.
+		// If `git rev-parse` is called on the branch name directly
+		// without using the fully qualified reference, then it would
+		// return the OID of the tag instead of the branch.
+		//
+		// In the past, this used to cause us to use the tag's OID as
+		// old revision when calling git-update-ref. As a result, the
+		// update would've failed as the branch's current revision
+		// didn't match the specified old revision.
+		testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath,
+			"branch", branchName,
+			"6d394385cf567f80a8fd85055db1ab4c5295806f")
+		testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "tag", branchName, "6d394385cf567f80a8fd85055db1ab4c5295806f~")
+
+		commitID := "cfe32cf61b73a0d5e9f13e774abde7ff789b1660"
+		request := &gitalypb.UserFFBranchRequest{
+			Repository: testRepo,
+			CommitId:   commitID,
+			Branch:     []byte(branchName),
+			User:       testhelper.TestUser,
+		}
+		expectedResponse := &gitalypb.UserFFBranchResponse{
+			BranchUpdate: &gitalypb.OperationBranchUpdate{
+				RepoCreated:   false,
+				BranchCreated: false,
+				CommitId:      commitID,
+			},
+		}
+
+		resp, err := client.UserFFBranch(ctx, request)
+		require.NoError(t, err)
+		testhelper.ProtoEqual(t, expectedResponse, resp)
+		newBranchHead := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "rev-parse", "refs/heads/"+branchName)
+		require.Equal(t, commitID, text.ChompBytes(newBranchHead), "branch head not updated")
+	})
+}
+
 func TestSuccessfulUserMergeToRefRequest(t *testing.T) {
 	ctx, cleanup := testhelper.Context()
 	defer cleanup()
diff --git a/proto/go/gitalypb/operations.pb.go b/proto/go/gitalypb/operations.pb.go
index 8ce862f77..017d79de7 100644
--- a/proto/go/gitalypb/operations.pb.go
+++ b/proto/go/gitalypb/operations.pb.go
@@ -1010,14 +1010,22 @@ func (m *OperationBranchUpdate) GetBranchCreated() bool {
 	return false
 }
 
+// UserFFBranchRequest contains parameters for the UserFFBranch RPC.
 type UserFFBranchRequest struct {
-	Repository           *Repository `protobuf:"bytes,1,opt,name=repository,proto3" json:"repository,omitempty"`
-	User                 *User       `protobuf:"bytes,2,opt,name=user,proto3" json:"user,omitempty"`
-	CommitId             string      `protobuf:"bytes,3,opt,name=commit_id,json=commitId,proto3" json:"commit_id,omitempty"`
-	Branch               []byte      `protobuf:"bytes,4,opt,name=branch,proto3" json:"branch,omitempty"`
-	XXX_NoUnkeyedLiteral struct{}    `json:"-"`
-	XXX_unrecognized     []byte      `json:"-"`
-	XXX_sizecache        int32       `json:"-"`
+	// repository is the repository for which to perform the fast-forward merge.
+	Repository *Repository `protobuf:"bytes,1,opt,name=repository,proto3" json:"repository,omitempty"`
+	// user is the user which to perform the fast-forward merge as. This is used
+	// for authorization checks.
+	User *User `protobuf:"bytes,2,opt,name=user,proto3" json:"user,omitempty"`
+	// commit_id is the commit ID to update the branch to.
+	CommitId string `protobuf:"bytes,3,opt,name=commit_id,json=commitId,proto3" json:"commit_id,omitempty"`
+	// branch is the name of the branch that shall be update. This must be the
+	// branch name only and not a fully qualified reference, e.g. "master"
+	// instead of "refs/heads/master".
+	Branch               []byte   `protobuf:"bytes,4,opt,name=branch,proto3" json:"branch,omitempty"`
+	XXX_NoUnkeyedLiteral struct{} `json:"-"`
+	XXX_unrecognized     []byte   `json:"-"`
+	XXX_sizecache        int32    `json:"-"`
 }
 
 func (m *UserFFBranchRequest) Reset()         { *m = UserFFBranchRequest{} }
@@ -2822,6 +2830,10 @@ type OperationServiceClient interface {
 	UserDeleteTag(ctx context.Context, in *UserDeleteTagRequest, opts ...grpc.CallOption) (*UserDeleteTagResponse, error)
 	UserMergeToRef(ctx context.Context, in *UserMergeToRefRequest, opts ...grpc.CallOption) (*UserMergeToRefResponse, error)
 	UserMergeBranch(ctx context.Context, opts ...grpc.CallOption) (OperationService_UserMergeBranchClient, error)
+	// UserFFBranch tries to perform a fast-forward merge of the given branch to
+	// the given commit. If the merge is not a fast-forward merge, the request
+	// will fail. The RPC will return an empty response in case updating the
+	// reference fails e.g. because of a race.
 	UserFFBranch(ctx context.Context, in *UserFFBranchRequest, opts ...grpc.CallOption) (*UserFFBranchResponse, error)
 	UserCherryPick(ctx context.Context, in *UserCherryPickRequest, opts ...grpc.CallOption) (*UserCherryPickResponse, error)
 	// UserCommitFiles builds a commit from a stream of actions and updates the target branch to point to it.
@@ -3082,6 +3094,10 @@ type OperationServiceServer interface {
 	UserDeleteTag(context.Context, *UserDeleteTagRequest) (*UserDeleteTagResponse, error)
 	UserMergeToRef(context.Context, *UserMergeToRefRequest) (*UserMergeToRefResponse, error)
 	UserMergeBranch(OperationService_UserMergeBranchServer) error
+	// UserFFBranch tries to perform a fast-forward merge of the given branch to
+	// the given commit. If the merge is not a fast-forward merge, the request
+	// will fail. The RPC will return an empty response in case updating the
+	// reference fails e.g. because of a race.
 	UserFFBranch(context.Context, *UserFFBranchRequest) (*UserFFBranchResponse, error)
 	UserCherryPick(context.Context, *UserCherryPickRequest) (*UserCherryPickResponse, error)
 	// UserCommitFiles builds a commit from a stream of actions and updates the target branch to point to it.
diff --git a/proto/operations.proto b/proto/operations.proto
index 741e43ad5..a91883679 100644
--- a/proto/operations.proto
+++ b/proto/operations.proto
@@ -43,11 +43,17 @@ service OperationService {
       op: MUTATOR
     };
   }
+
+  // UserFFBranch tries to perform a fast-forward merge of the given branch to
+  // the given commit. If the merge is not a fast-forward merge, the request
+  // will fail. The RPC will return an empty response in case updating the
+  // reference fails e.g. because of a race.
   rpc UserFFBranch(UserFFBranchRequest) returns (UserFFBranchResponse) {
     option (op_type) = {
       op: MUTATOR
     };
   }
+
   rpc UserCherryPick(UserCherryPickRequest) returns (UserCherryPickResponse) {
     option (op_type) = {
       op: MUTATOR
@@ -56,8 +62,8 @@ service OperationService {
 
   // UserCommitFiles builds a commit from a stream of actions and updates the target branch to point to it.
   // UserCommitFilesRequest with a UserCommitFilesRequestHeader must be sent as the first message of the stream.
-  // Following that, a variable number of actions can be sent to build a new commit. Each action consists of 
-  // a header followed by content if used by the action. 
+  // Following that, a variable number of actions can be sent to build a new commit. Each action consists of
+  // a header followed by content if used by the action.
   rpc UserCommitFiles(stream UserCommitFilesRequest) returns (UserCommitFilesResponse) {
     option (op_type) = {
       op: MUTATOR
@@ -203,17 +209,25 @@ message OperationBranchUpdate {
   // commit_id is set to the OID of the created commit if a branch was created or updated.
   string commit_id = 1;
   // repo_created indicates whether the branch created was the first one in the repository.
-  // Used for cache invalidation in GitLab. 
+  // Used for cache invalidation in GitLab.
   bool repo_created = 2;
   // branch_created indicates whether the branch already existed in the repository
-  // and was updated or whether it was created. Used for cache invalidation in GitLab. 
+  // and was updated or whether it was created. Used for cache invalidation in GitLab.
   bool branch_created = 3;
 }
 
+// UserFFBranchRequest contains parameters for the UserFFBranch RPC.
 message UserFFBranchRequest {
+  // repository is the repository for which to perform the fast-forward merge.
   Repository repository = 1 [(target_repository)=true];
+  // user is the user which to perform the fast-forward merge as. This is used
+  // for authorization checks.
   User user = 2;
+  // commit_id is the commit ID to update the branch to.
   string commit_id = 3;
+  // branch is the name of the branch that shall be update. This must be the
+  // branch name only and not a fully qualified reference, e.g. "master"
+  // instead of "refs/heads/master".
   bytes branch = 4;
 }
 
@@ -328,7 +342,7 @@ message UserCommitFilesAction {
 }
 
 // UserCommitFilesRequestHeader is the header of the UserCommitFiles that defines the commit details,
-// parent and other information related to the call. 
+// parent and other information related to the call.
 message UserCommitFilesRequestHeader {
   // repository is the target repository where to apply the commit.
   Repository repository = 1 [(target_repository)=true];
@@ -346,7 +360,7 @@ message UserCommitFilesRequestHeader {
   // used instead.
   bytes commit_author_email = 6;
   // start_branch_name specifies the branch whose commit to use as the parent commit. Takes priority
-  // over branch_name. Optional. 
+  // over branch_name. Optional.
   bytes start_branch_name = 7;
   // start_repository specifies which contains the parent commit. If not specified, repository itself
   // is used to look up the parent commit. Optional.
@@ -355,7 +369,7 @@ message UserCommitFilesRequestHeader {
   // point to the new commit.
   bool force = 9;
   // start_sha specifies the SHA of the commit to use as the parent of new commit. Takes priority
-  // over start_branch_name and branc_name. Optional. 
+  // over start_branch_name and branc_name. Optional.
   string start_sha = 10;
 }
 
@@ -374,7 +388,7 @@ message UserCommitFilesRequest {
 message UserCommitFilesResponse {
   // branch_update contains the details of the commit and the branch update.
   OperationBranchUpdate branch_update = 1;
-  // index_error is set to the error message when an invalid action was attempted, such as 
+  // index_error is set to the error message when an invalid action was attempted, such as
   // trying to create a file that already existed.
   string index_error = 2;
   // pre_receive_error is set when the pre-receive hook errored.
diff --git a/ruby/proto/gitaly/operations_services_pb.rb b/ruby/proto/gitaly/operations_services_pb.rb
index 3c98fd830..66b095541 100644
--- a/ruby/proto/gitaly/operations_services_pb.rb
+++ b/ruby/proto/gitaly/operations_services_pb.rb
@@ -21,12 +21,16 @@ module Gitaly
       rpc :UserDeleteTag, Gitaly::UserDeleteTagRequest, Gitaly::UserDeleteTagResponse
       rpc :UserMergeToRef, Gitaly::UserMergeToRefRequest, Gitaly::UserMergeToRefResponse
       rpc :UserMergeBranch, stream(Gitaly::UserMergeBranchRequest), stream(Gitaly::UserMergeBranchResponse)
+      # UserFFBranch tries to perform a fast-forward merge of the given branch to
+      # the given commit. If the merge is not a fast-forward merge, the request
+      # will fail. The RPC will return an empty response in case updating the
+      # reference fails e.g. because of a race.
       rpc :UserFFBranch, Gitaly::UserFFBranchRequest, Gitaly::UserFFBranchResponse
       rpc :UserCherryPick, Gitaly::UserCherryPickRequest, Gitaly::UserCherryPickResponse
       # UserCommitFiles builds a commit from a stream of actions and updates the target branch to point to it.
       # UserCommitFilesRequest with a UserCommitFilesRequestHeader must be sent as the first message of the stream.
-      # Following that, a variable number of actions can be sent to build a new commit. Each action consists of 
-      # a header followed by content if used by the action. 
+      # Following that, a variable number of actions can be sent to build a new commit. Each action consists of
+      # a header followed by content if used by the action.
       rpc :UserCommitFiles, stream(Gitaly::UserCommitFilesRequest), Gitaly::UserCommitFilesResponse
       rpc :UserRebaseConfirmable, stream(Gitaly::UserRebaseConfirmableRequest), stream(Gitaly::UserRebaseConfirmableResponse)
       rpc :UserRevert, Gitaly::UserRevertRequest, Gitaly::UserRevertResponse