Add OperationService.UserUpdateBranch RPC

9 files changed, 287 insertions, 45 deletions

diff --git a/changelogs/unreleased/1257-server-implementation-operationservice-userupdatebranch.yml b/changelogs/unreleased/1257-server-implementation-operationservice-userupdatebranch.yml
new file mode 100644
index 000000000..a1cc5fdfc
--- /dev/null
+++ b/changelogs/unreleased/1257-server-implementation-operationservice-userupdatebranch.yml
@@ -0,0 +1,5 @@
+---
+title: Add OperationService.UserUpdateBranch RPC
+merge_request: 778
+author:
+type: added
diff --git a/internal/service/operations/branches.go b/internal/service/operations/branches.go
index f10d100ce..c6750872c 100644
--- a/internal/service/operations/branches.go
+++ b/internal/service/operations/branches.go
@@ -1,7 +1,6 @@
 package operations
 
 import (
-	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/internal/rubyserver"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
@@ -26,7 +25,17 @@ func (s *server) UserCreateBranch(ctx context.Context, req *pb.UserCreateBranchR
 }
 
 func (s *server) UserUpdateBranch(ctx context.Context, req *pb.UserUpdateBranchRequest) (*pb.UserUpdateBranchResponse, error) {
-	return nil, helper.Unimplemented
+	client, err := s.OperationServiceClient(ctx)
+	if err != nil {
+		return nil, err
+	}
+
+	clientCtx, err := rubyserver.SetHeaders(ctx, req.GetRepository())
+	if err != nil {
+		return nil, err
+	}
+
+	return client.UserUpdateBranch(clientCtx, req)
 }
 
 func (s *server) UserDeleteBranch(ctx context.Context, req *pb.UserDeleteBranchRequest) (*pb.UserDeleteBranchResponse, error) {
diff --git a/internal/service/operations/branches_test.go b/internal/service/operations/branches_test.go
index 80ffcb245..4a44ed9b4 100644
--- a/internal/service/operations/branches_test.go
+++ b/internal/service/operations/branches_test.go
@@ -33,11 +33,6 @@ func TestSuccessfulUserCreateBranchRequest(t *testing.T) {
 	startPoint := "c7fbe50c7c7419d9701eebe64b1fdacc3df5b9dd"
 	startPointCommit, err := log.GetCommit(ctx, testRepo, startPoint)
 	require.NoError(t, err)
-	user := &pb.User{
-		Name:  []byte("Alejandro Rodríguez"),
-		Email: []byte("alejandro@gitlab.com"),
-		GlId:  "user-1",
-	}
 
 	testCases := []struct {
 		desc           string
@@ -95,12 +90,6 @@ func TestSuccessfulGitHooksForUserCreateBranchRequest(t *testing.T) {
 	defer conn.Close()
 
 	branchName := "new-branch"
-	user := &pb.User{
-		Name:       []byte("Alejandro Rodríguez"),
-		Email:      []byte("alejandro@gitlab.com"),
-		GlId:       "user-1",
-		GlUsername: "johndoe",
-	}
 	request := &pb.UserCreateBranchRequest{
 		Repository: testRepo,
 		BranchName: []byte(branchName),
@@ -140,12 +129,6 @@ func TestFailedUserCreateBranchDueToHooks(t *testing.T) {
 	client, conn := newOperationClient(t, serverSocketPath)
 	defer conn.Close()
 
-	user := &pb.User{
-		Name:       []byte("Alejandro Rodríguez"),
-		Email:      []byte("alejandro@gitlab.com"),
-		GlId:       "user-1",
-		GlUsername: "johndoe",
-	}
 	request := &pb.UserCreateBranchRequest{
 		Repository: testRepo,
 		BranchName: []byte("new-branch"),
@@ -184,10 +167,6 @@ func TestFailedUserCreateBranchRequest(t *testing.T) {
 	testRepo, _, cleanupFn := testhelper.NewTestRepo(t)
 	defer cleanupFn()
 
-	user := &pb.User{
-		Name:  []byte("Alejandro Rodríguez"),
-		Email: []byte("alejandro@gitlab.com"),
-	}
 	testCases := []struct {
 		desc       string
 		branchName string
diff --git a/internal/service/operations/squash_test.go b/internal/service/operations/squash_test.go
index 53781fe3e..94b599f5f 100644
--- a/internal/service/operations/squash_test.go
+++ b/internal/service/operations/squash_test.go
@@ -11,11 +11,6 @@ import (
 )
 
 var (
-	user = &pb.User{
-		Name:  []byte("Jane Doe"),
-		Email: []byte("janedoe@gitlab.com"),
-		GlId:  "user-123",
-	}
 	author = &pb.User{
 		Name:  []byte("John Doe"),
 		Email: []byte("johndoe@gitlab.com"),
diff --git a/internal/service/operations/testhelper_test.go b/internal/service/operations/testhelper_test.go
index 076f85e8f..182ccd97c 100644
--- a/internal/service/operations/testhelper_test.go
+++ b/internal/service/operations/testhelper_test.go
@@ -26,6 +26,12 @@ var (
 	GitlabPreHooks  = gitlabPreHooks
 	GitlabHooks     []string
 	RubyServer      *rubyserver.Server
+	user            = &pb.User{
+		Name:       []byte("Jane Doe"),
+		Email:      []byte("janedoe@gitlab.com"),
+		GlId:       "user-123",
+		GlUsername: "janedoe",
+	}
 )
 
 func init() {
diff --git a/internal/service/operations/update_branches_test.go b/internal/service/operations/update_branches_test.go
new file mode 100644
index 000000000..bd6061f2b
--- /dev/null
+++ b/internal/service/operations/update_branches_test.go
@@ -0,0 +1,225 @@
+package operations
+
+import (
+	"io/ioutil"
+	"os"
+	"path"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	pb "gitlab.com/gitlab-org/gitaly-proto/go"
+	"gitlab.com/gitlab-org/gitaly/internal/git/log"
+	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
+	"golang.org/x/net/context"
+	"google.golang.org/grpc/codes"
+)
+
+var (
+	updateBranchName = "feature"
+	newrev           = []byte("1a35b5a77cf6af7edf6703f88e82f6aff613666f")
+	oldrev           = []byte("0b4bc9a49b562e85de7cc9e834518ea6828729b9")
+)
+
+func TestSuccessfulUserUpdateBranchRequest(t *testing.T) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	testRepo, _, cleanupFn := testhelper.NewTestRepo(t)
+	defer cleanupFn()
+
+	server, serverSocketPath := runOperationServiceServer(t)
+	defer server.Stop()
+
+	client, conn := newOperationClient(t, serverSocketPath)
+	defer conn.Close()
+
+	request := &pb.UserUpdateBranchRequest{
+		Repository: testRepo,
+		BranchName: []byte(updateBranchName),
+		Newrev:     newrev,
+		Oldrev:     oldrev,
+		User:       user,
+	}
+
+	response, err := client.UserUpdateBranch(ctx, request)
+
+	require.NoError(t, err)
+	require.Empty(t, response.PreReceiveError)
+
+	branchCommit, err := log.GetCommit(ctx, testRepo, updateBranchName)
+
+	require.NoError(t, err)
+	require.Equal(t, string(newrev), branchCommit.Id)
+}
+
+func TestSuccessfulGitHooksForUserUpdateBranchRequest(t *testing.T) {
+	server, serverSocketPath := runOperationServiceServer(t)
+	defer server.Stop()
+
+	client, conn := newOperationClient(t, serverSocketPath)
+	defer conn.Close()
+
+	for _, hookName := range GitlabHooks {
+		t.Run(hookName, func(t *testing.T) {
+			testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
+			defer cleanupFn()
+
+			hookPath, hookOutputTempPath := WriteEnvToHook(t, testRepoPath, hookName)
+			defer os.Remove(hookPath)
+			defer os.Remove(hookOutputTempPath)
+
+			ctx, cancel := testhelper.Context()
+			defer cancel()
+
+			request := &pb.UserUpdateBranchRequest{
+				Repository: testRepo,
+				BranchName: []byte(updateBranchName),
+				Newrev:     newrev,
+				Oldrev:     oldrev,
+				User:       user,
+			}
+
+			response, err := client.UserUpdateBranch(ctx, request)
+			require.NoError(t, err)
+			require.Empty(t, response.PreReceiveError)
+
+			output := string(testhelper.MustReadFile(t, hookOutputTempPath))
+			require.Contains(t, output, "GL_ID="+user.GlId)
+			require.Contains(t, output, "GL_USERNAME="+user.GlUsername)
+		})
+	}
+}
+
+func TestFailedUserUpdateBranchDueToHooks(t *testing.T) {
+	testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
+	defer cleanupFn()
+
+	server, serverSocketPath := runOperationServiceServer(t)
+	defer server.Stop()
+
+	client, conn := newOperationClient(t, serverSocketPath)
+	defer conn.Close()
+
+	request := &pb.UserUpdateBranchRequest{
+		Repository: testRepo,
+		BranchName: []byte(updateBranchName),
+		Newrev:     newrev,
+		Oldrev:     oldrev,
+		User:       user,
+	}
+	// Write a hook that will fail with the environment as the error message
+	// so we can check that string for our env variables.
+	hookContent := []byte("#!/bin/sh\nprintenv | paste -sd ' ' -\nexit 1")
+
+	for _, hookName := range gitlabPreHooks {
+		hookPath := path.Join(testRepoPath, "hooks", hookName)
+		ioutil.WriteFile(hookPath, hookContent, 0755)
+		defer os.Remove(hookPath)
+
+		ctx, cancel := context.WithCancel(context.Background())
+		defer cancel()
+
+		response, err := client.UserUpdateBranch(ctx, request)
+		require.Nil(t, err)
+		require.Contains(t, response.PreReceiveError, "GL_ID="+user.GlId)
+		require.Contains(t, response.PreReceiveError, "GL_USERNAME="+user.GlUsername)
+		require.Contains(t, response.PreReceiveError, "GL_REPOSITORY="+testRepo.GlRepository)
+		require.Contains(t, response.PreReceiveError, "GL_PROTOCOL=web")
+		require.Contains(t, response.PreReceiveError, "PWD="+testRepoPath)
+	}
+}
+
+func TestFailedUserUpdateBranchRequest(t *testing.T) {
+	server, serverSocketPath := runOperationServiceServer(t)
+	defer server.Stop()
+
+	client, conn := newOperationClient(t, serverSocketPath)
+	defer conn.Close()
+
+	testRepo, _, cleanupFn := testhelper.NewTestRepo(t)
+	defer cleanupFn()
+
+	testCases := []struct {
+		desc       string
+		branchName string
+		newrev     []byte
+		oldrev     []byte
+		user       *pb.User
+		code       codes.Code
+	}{
+		{
+			desc:       "empty branch name",
+			branchName: "",
+			newrev:     newrev,
+			oldrev:     oldrev,
+			user:       user,
+			code:       codes.InvalidArgument,
+		},
+		{
+			desc:       "empty newrev",
+			branchName: updateBranchName,
+			newrev:     nil,
+			oldrev:     oldrev,
+			user:       user,
+			code:       codes.InvalidArgument,
+		},
+		{
+			desc:       "empty oldrev",
+			branchName: updateBranchName,
+			newrev:     newrev,
+			oldrev:     nil,
+			user:       user,
+			code:       codes.InvalidArgument,
+		},
+		{
+			desc:       "empty user",
+			branchName: updateBranchName,
+			newrev:     newrev,
+			oldrev:     oldrev,
+			user:       nil,
+			code:       codes.InvalidArgument,
+		},
+		{
+			desc:       "non-existing branch",
+			branchName: "i-dont-exist",
+			newrev:     newrev,
+			oldrev:     oldrev,
+			user:       user,
+			code:       codes.FailedPrecondition,
+		},
+		{
+			desc:       "non-existing newrev",
+			branchName: updateBranchName,
+			newrev:     []byte("i-dont-exist"),
+			oldrev:     oldrev,
+			user:       user,
+			code:       codes.FailedPrecondition,
+		},
+		{
+			desc:       "non-existing oldrev",
+			branchName: updateBranchName,
+			newrev:     newrev,
+			oldrev:     []byte("i-dont-exist"),
+			user:       user,
+			code:       codes.FailedPrecondition,
+		},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.desc, func(t *testing.T) {
+			request := &pb.UserUpdateBranchRequest{
+				Repository: testRepo,
+				BranchName: []byte(testCase.branchName),
+				Newrev:     testCase.newrev,
+				Oldrev:     testCase.oldrev,
+				User:       testCase.user,
+			}
+
+			ctx, cancel := context.WithCancel(context.Background())
+			defer cancel()
+
+			_, err := client.UserUpdateBranch(ctx, request)
+			testhelper.RequireGrpcError(t, err, testCase.code)
+		})
+	}
+}
diff --git a/ruby/Gemfile b/ruby/Gemfile
index 05295a63b..7ad977550 100644
--- a/ruby/Gemfile
+++ b/ruby/Gemfile
@@ -3,7 +3,7 @@ source 'https://rubygems.org'
 gem 'rugged', '~> 0.27.2'
 gem 'github-linguist', '~> 5.3.3', require: 'linguist'
 gem 'gitlab-markup', '~> 1.6.2'
-gem 'gitaly-proto', '~> 0.101.0', require: 'gitaly'
+gem 'gitaly-proto', '~> 0.104.0', require: 'gitaly'
 gem 'activesupport', '~> 5.0.2'
 gem 'rdoc', '~> 4.2'
 gem 'gitlab-gollum-lib', '~> 4.2', require: false
diff --git a/ruby/Gemfile.lock b/ruby/Gemfile.lock
index 13bcf704b..2f1b0d6f7 100644
--- a/ruby/Gemfile.lock
+++ b/ruby/Gemfile.lock
@@ -17,7 +17,7 @@ GEM
       multipart-post (>= 1.2, < 3)
     gemojione (3.3.0)
       json
-    gitaly-proto (0.101.0)
+    gitaly-proto (0.104.0)
       google-protobuf (~> 3.1)
       grpc (~> 1.10)
     github-linguist (5.3.3)
@@ -142,7 +142,7 @@ PLATFORMS
 DEPENDENCIES
   activesupport (~> 5.0.2)
   faraday (~> 0.12)
-  gitaly-proto (~> 0.101.0)
+  gitaly-proto (~> 0.104.0)
   github-linguist (~> 5.3.3)
   gitlab-gollum-lib (~> 4.2)
   gitlab-gollum-rugged_adapter (~> 0.4.4)
diff --git a/ruby/lib/gitaly_server/operations_service.rb b/ruby/lib/gitaly_server/operations_service.rb
index 5a56a8451..362e50d65 100644
--- a/ruby/lib/gitaly_server/operations_service.rb
+++ b/ruby/lib/gitaly_server/operations_service.rb
@@ -7,15 +7,12 @@ module GitalyServer
         begin
           repo = Gitlab::Git::Repository.from_gitaly(request.repository, call)
 
-          gitaly_user = request.user
-          raise GRPC::InvalidArgument.new('empty user') unless gitaly_user
+          gitaly_user = get_param!(request, :user)
           user = Gitlab::Git::User.from_gitaly(gitaly_user)
 
-          tag_name = request.tag_name
-          raise GRPC::InvalidArgument.new('empty tag name') unless tag_name.present?
+          tag_name = get_param!(request, :tag_name)
 
-          target_revision = request.target_revision
-          raise GRPC::InvalidArgument.new('empty target revision') unless target_revision.present?
+          target_revision = get_param!(request, :target_revision)
 
           created_tag = repo.add_tag(tag_name, user: user, target: target_revision, message: request.message.presence)
           return Gitaly::UserCreateTagResponse.new unless created_tag
@@ -40,12 +37,10 @@ module GitalyServer
         begin
           repo = Gitlab::Git::Repository.from_gitaly(request.repository, call)
 
-          gitaly_user = request.user
-          raise GRPC::InvalidArgument.new('empty user') unless gitaly_user
+          gitaly_user = get_param!(request, :user)
           user = Gitlab::Git::User.from_gitaly(gitaly_user)
 
-          tag_name = request.tag_name
-          raise GRPC::InvalidArgument.new('empty tag name') if tag_name.blank?
+          tag_name = get_param!(request, :tag_name)
 
           repo.rm_tag(tag_name, user: user)
 
@@ -62,10 +57,8 @@ module GitalyServer
       bridge_exceptions do
         begin
           repo = Gitlab::Git::Repository.from_gitaly(request.repository, call)
-          target = request.start_point
-          raise GRPC::InvalidArgument.new('empty start_point') if target.empty?
-          gitaly_user = request.user
-          raise GRPC::InvalidArgument.new('empty user') unless gitaly_user
+          target = get_param!(request, :start_point)
+          gitaly_user = get_param!(request, :user)
 
           branch_name = request.branch_name
           user = Gitlab::Git::User.from_gitaly(gitaly_user)
@@ -84,6 +77,27 @@ module GitalyServer
       end
     end
 
+    def user_update_branch(request, call)
+      bridge_exceptions do
+        begin
+          repo = Gitlab::Git::Repository.from_gitaly(request.repository, call)
+          branch_name = get_param!(request, :branch_name)
+          newrev = get_param!(request, :newrev)
+          oldrev = get_param!(request, :oldrev)
+          gitaly_user = get_param!(request, :user)
+
+          user = Gitlab::Git::User.from_gitaly(gitaly_user)
+          repo.update_branch(branch_name, user: user, newrev: newrev, oldrev: oldrev)
+
+          Gitaly::UserUpdateBranchResponse.new
+        rescue Gitlab::Git::Repository::InvalidRef, Gitlab::Git::CommitError => ex
+          raise GRPC::FailedPrecondition.new(ex.message)
+        rescue Gitlab::Git::PreReceiveError => ex
+          return Gitaly::UserUpdateBranchResponse.new(pre_receive_error: set_utf8!(ex.message))
+        end
+      end
+    end
+
     def user_delete_branch(request, call)
       bridge_exceptions do
         begin
@@ -331,5 +345,14 @@ module GitalyServer
         branch_created: gitlab_update_result.branch_created
       )
     end
+
+    def get_param!(request, name)
+      value = request[name.to_s]
+
+      return value if value.present?
+
+      field_name = name.to_s.tr('_', ' ')
+      raise GRPC::InvalidArgument.new("empty #{field_name}")
+    end
   end
 end