Support ref path in merge_to_ref

12 files changed, 100 insertions, 50 deletions

diff --git a/changelogs/unreleased/support-ref-in-merge-to-ref.yml b/changelogs/unreleased/support-ref-in-merge-to-ref.yml
new file mode 100644
index 000000000..8aaa4a7fc
--- /dev/null
+++ b/changelogs/unreleased/support-ref-in-merge-to-ref.yml
@@ -0,0 +1,5 @@
+---
+title: Add support first_parent_ref in UserMergeToRef
+merge_request: 1210
+author:
+type: changed
diff --git a/go.mod b/go.mod
index ef83ae147..f1e0b072e 100644
--- a/go.mod
+++ b/go.mod
@@ -14,7 +14,7 @@ require (
 	github.com/sirupsen/logrus v1.2.0
 	github.com/stretchr/testify v1.3.0
 	github.com/tinylib/msgp v1.1.0 // indirect
-	gitlab.com/gitlab-org/gitaly-proto v1.33.0
+	gitlab.com/gitlab-org/gitaly-proto v1.36.0
 	gitlab.com/gitlab-org/labkit v0.0.0-20190221122536-0c3fc7cdd57c
 	golang.org/x/net v0.0.0-20190613194153-d28f0bde5980
 	golang.org/x/sync v0.0.0-20181221193216-37e7f081c4d4
diff --git a/go.sum b/go.sum
index 3fd2dc676..3e59959df 100644
--- a/go.sum
+++ b/go.sum
@@ -103,8 +103,8 @@ github.com/uber/jaeger-client-go v2.15.0+incompatible h1:NP3qsSqNxh8VYr956ur1N/1
 github.com/uber/jaeger-client-go v2.15.0+incompatible/go.mod h1:WVhlPFC8FDjOFMMWRy2pZqQJSXxYSwNYOkTr/Z6d3Kk=
 github.com/uber/jaeger-lib v1.5.0 h1:OHbgr8l656Ub3Fw5k9SWnBfIEwvoHQ+W2y+Aa9D1Uyo=
 github.com/uber/jaeger-lib v1.5.0/go.mod h1:ComeNDZlWwrWnDv8aPp0Ba6+uUTzImX/AauajbLI56U=
-gitlab.com/gitlab-org/gitaly-proto v1.33.0 h1:gSwV1hGpwrEauYcl81j214DRfUHAznBeeOMdwbvadnc=
-gitlab.com/gitlab-org/gitaly-proto v1.33.0/go.mod h1:zNjk/86bjwLVJ4NcvInBcXcLdptdRFQ28sYrdFbrFgY=
+gitlab.com/gitlab-org/gitaly-proto v1.36.0 h1:TnWEwCwq/1epAcz1VTAiUMhlKYpnlAV8X1CO1Jwb8hE=
+gitlab.com/gitlab-org/gitaly-proto v1.36.0/go.mod h1:zNjk/86bjwLVJ4NcvInBcXcLdptdRFQ28sYrdFbrFgY=
 gitlab.com/gitlab-org/labkit v0.0.0-20190221122536-0c3fc7cdd57c h1:xo48LcGsTCasKcJpQDBCCuZU+aP8uGaboUVvD7Lgm6g=
 gitlab.com/gitlab-org/labkit v0.0.0-20190221122536-0c3fc7cdd57c/go.mod h1:rYhLgfrbEcyfinG+R3EvKu6bZSsmwQqcXzLfHWSfUKM=
 go.uber.org/atomic v1.3.2 h1:2Oa65PReHzfn29GpvgsYwloV9AVFHPDk8tYxt2c2tr4=
diff --git a/internal/service/operations/merge.go b/internal/service/operations/merge.go
index dd44a76be..15ae06677 100644
--- a/internal/service/operations/merge.go
+++ b/internal/service/operations/merge.go
@@ -92,8 +92,8 @@ func (s *server) UserFFBranch(ctx context.Context, in *gitalypb.UserFFBranchRequ
 }
 
 func validateUserMergeToRefRequest(in *gitalypb.UserMergeToRefRequest) error {
-	if len(in.Branch) == 0 {
-		return fmt.Errorf("empty branch name")
+	if len(in.FirstParentRef) == 0 && len(in.Branch) == 0 {
+		return fmt.Errorf("empty first parent ref and branch name")
 	}
 
 	if in.User == nil {
diff --git a/internal/service/operations/merge_test.go b/internal/service/operations/merge_test.go
index b9a0be004..2492fe8ec 100644
--- a/internal/service/operations/merge_test.go
+++ b/internal/service/operations/merge_test.go
@@ -475,30 +475,40 @@ func TestSuccessfulUserMergeToRefRequest(t *testing.T) {
 	require.NoError(t, err, "give an existing state to the target ref: %s", out)
 
 	testCases := []struct {
-		desc      string
-		user      *gitalypb.User
-		branch    []byte
-		targetRef []byte
-		emptyRef  bool
-		sourceSha string
-		message   string
+		desc           string
+		user           *gitalypb.User
+		branch         []byte
+		targetRef      []byte
+		emptyRef       bool
+		sourceSha      string
+		message        string
+		firstParentRef []byte
 	}{
 		{
-			desc:      "empty target ref merge",
-			user:      mergeUser,
-			branch:    []byte(mergeBranchName),
-			targetRef: emptyTargetRef,
-			emptyRef:  true,
-			sourceSha: commitToMerge,
-			message:   mergeCommitMessage,
+			desc:           "empty target ref merge",
+			user:           mergeUser,
+			targetRef:      emptyTargetRef,
+			emptyRef:       true,
+			sourceSha:      commitToMerge,
+			message:        mergeCommitMessage,
+			firstParentRef: []byte("refs/heads/" + mergeBranchName),
 		},
 		{
-			desc:      "existing target ref",
+			desc:           "existing target ref",
+			user:           mergeUser,
+			targetRef:      existingTargetRef,
+			emptyRef:       false,
+			sourceSha:      commitToMerge,
+			message:        mergeCommitMessage,
+			firstParentRef: []byte("refs/heads/" + mergeBranchName),
+		},
+		{
+			desc:      "branch is specified and firstParentRef is empty",
 			user:      mergeUser,
 			branch:    []byte(mergeBranchName),
 			targetRef: existingTargetRef,
 			emptyRef:  false,
-			sourceSha: commitToMerge,
+			sourceSha: "38008cb17ce1466d8fec2dfa6f6ab8dcfe5cf49e",
 			message:   mergeCommitMessage,
 		},
 	}
@@ -509,12 +519,13 @@ func TestSuccessfulUserMergeToRefRequest(t *testing.T) {
 			defer cancel()
 
 			request := &gitalypb.UserMergeToRefRequest{
-				Repository: testRepo,
-				User:       testCase.user,
-				Branch:     testCase.branch,
-				TargetRef:  testCase.targetRef,
-				SourceSha:  testCase.sourceSha,
-				Message:    []byte(testCase.message),
+				Repository:     testRepo,
+				User:           testCase.user,
+				Branch:         testCase.branch,
+				TargetRef:      testCase.targetRef,
+				SourceSha:      testCase.sourceSha,
+				Message:        []byte(testCase.message),
+				FirstParentRef: testCase.firstParentRef,
 			}
 
 			commitBeforeRefMerge, fetchRefBeforeMergeErr := gitlog.GetCommit(ctx, testRepo, string(testCase.targetRef))
@@ -608,7 +619,7 @@ func TestFailedUserMergeToRefRequest(t *testing.T) {
 			code:      codes.InvalidArgument,
 		},
 		{
-			desc:      "empty branch",
+			desc:      "empty branch and first parent ref",
 			repo:      testRepo,
 			user:      mergeUser,
 			sourceSha: commitToMerge,
diff --git a/internal/service/repository/server.go b/internal/service/repository/server.go
index a4a0355e9..dd8092af8 100644
--- a/internal/service/repository/server.go
+++ b/internal/service/repository/server.go
@@ -20,3 +20,11 @@ func NewServer(rs *rubyserver.Server) gitalypb.RepositoryServiceServer {
 func (*server) FetchHTTPRemote(context.Context, *gitalypb.FetchHTTPRemoteRequest) (*gitalypb.FetchHTTPRemoteResponse, error) {
 	return nil, helper.Unimplemented
 }
+
+func (*server) CloneFromPool(context.Context, *gitalypb.CloneFromPoolRequest) (*gitalypb.CloneFromPoolResponse, error) {
+	return nil, helper.Unimplemented
+}
+
+func (*server) CloneFromPoolInternal(context.Context, *gitalypb.CloneFromPoolInternalRequest) (*gitalypb.CloneFromPoolInternalResponse, error) {
+	return nil, helper.Unimplemented
+}
diff --git a/ruby/Gemfile b/ruby/Gemfile
index 1ad7f4c77..1a6eb99b6 100644
--- a/ruby/Gemfile
+++ b/ruby/Gemfile
@@ -7,7 +7,7 @@ gem 'rugged', '~> 0.28'
 gem 'github-linguist', '~> 6.1', require: 'linguist'
 gem 'gitlab-markup', '~> 1.7.0'
 gem 'activesupport', '~> 5.1.7'
-gem 'gitaly-proto', '~> 1.32.0'
+gem 'gitaly-proto', '~> 1.36.0'
 gem 'rdoc', '~> 4.2'
 gem 'gitlab-gollum-lib', '~> 4.2.7.7', require: false
 gem 'gitlab-gollum-rugged_adapter', '~> 0.4.4.2', require: false
diff --git a/ruby/Gemfile.lock b/ruby/Gemfile.lock
index 852b3f332..604b48429 100644
--- a/ruby/Gemfile.lock
+++ b/ruby/Gemfile.lock
@@ -49,7 +49,7 @@ GEM
     ffi (1.10.0)
     gemojione (3.3.0)
       json
-    gitaly-proto (1.32.0)
+    gitaly-proto (1.36.0)
       grpc (~> 1.0)
     github-linguist (6.4.1)
       charlock_holmes (~> 0.7.6)
@@ -217,7 +217,7 @@ DEPENDENCIES
   bundler (>= 1.17.3)
   factory_bot
   faraday (~> 0.12)
-  gitaly-proto (~> 1.32.0)
+  gitaly-proto (~> 1.36.0)
   github-linguist (~> 6.1)
   gitlab-gollum-lib (~> 4.2.7.7)
   gitlab-gollum-rugged_adapter (~> 0.4.4.2)
diff --git a/ruby/lib/gitaly_server/operations_service.rb b/ruby/lib/gitaly_server/operations_service.rb
index 4243b77c7..89655fb3f 100644
--- a/ruby/lib/gitaly_server/operations_service.rb
+++ b/ruby/lib/gitaly_server/operations_service.rb
@@ -99,7 +99,7 @@ module GitalyServer
       repo = Gitlab::Git::Repository.from_gitaly(request.repository, call)
       user = Gitlab::Git::User.from_gitaly(request.user)
 
-      commit_id = repo.merge_to_ref(user, request.source_sha, request.branch, request.target_ref, request.message.dup)
+      commit_id = repo.merge_to_ref(user, request.source_sha, request.branch, request.target_ref, request.message.dup, request.first_parent_ref)
 
       Gitaly::UserMergeToRefResponse.new(commit_id: commit_id)
     rescue Gitlab::Git::CommitError => e
diff --git a/ruby/lib/gitlab/git/operation_service.rb b/ruby/lib/gitlab/git/operation_service.rb
index 6530ef342..f011f1aaa 100644
--- a/ruby/lib/gitlab/git/operation_service.rb
+++ b/ruby/lib/gitlab/git/operation_service.rb
@@ -107,18 +107,18 @@ module Gitlab
       # Returns the generated commit.
       #
       # ref - The target ref path we're committing to.
-      # from_branch - The branch we're taking the HEAD commit from.
-      def commit_ref(ref, from_branch:)
+      # from_ref - The ref we're taking the HEAD commit from.
+      def commit_ref(ref, from_ref:)
         update_autocrlf_option
 
-        repository.write_ref(ref, from_branch.target)
+        repository.write_ref(ref, from_ref.target)
 
         # Make commit
         newrev = yield
 
         raise Gitlab::Git::CommitError.new('Failed to create commit') unless newrev
 
-        oldrev = from_branch.target
+        oldrev = from_ref.target
 
         update_ref(ref, newrev, oldrev)
 
diff --git a/ruby/lib/gitlab/git/repository.rb b/ruby/lib/gitlab/git/repository.rb
index b7b1797ab..66b545084 100644
--- a/ruby/lib/gitlab/git/repository.rb
+++ b/ruby/lib/gitlab/git/repository.rb
@@ -267,13 +267,17 @@ module Gitlab
         tags.find { |tag| tag.name.b == name_b }
       end
 
-      def merge_to_ref(user, source_sha, branch, target_ref, message)
-        branch = find_branch(branch)
+      def merge_to_ref(user, source_sha, branch, target_ref, message, first_parent_ref)
+        ref = if first_parent_ref.present?
+                find_ref(first_parent_ref)
+              else
+                find_branch(branch)
+              end
 
-        raise InvalidRef unless branch
+        raise InvalidRef unless ref
 
-        OperationService.new(user, self).commit_ref(target_ref, from_branch: branch) do
-          our_commit = branch.target
+        OperationService.new(user, self).commit_ref(target_ref, from_ref: ref) do
+          our_commit = ref.target
           their_commit = source_sha
 
           create_merge_commit(user, our_commit, their_commit, message)
@@ -548,6 +552,14 @@ module Gitlab
         end
       end
 
+      def find_ref(name)
+        rugged_ref = rugged.references[name]
+
+        return unless rugged_ref
+
+        Gitlab::Git::Ref.new(self, rugged_ref.name, rugged_ref.target, rugged_ref.target_id)
+      end
+
       # Delete the specified branch from the repository
       def delete_branch(branch_name)
         rugged.branches.delete(branch_name)
diff --git a/ruby/spec/lib/gitlab/git/repository_spec.rb b/ruby/spec/lib/gitlab/git/repository_spec.rb
index 6c056efc8..672c95361 100644
--- a/ruby/spec/lib/gitlab/git/repository_spec.rb
+++ b/ruby/spec/lib/gitlab/git/repository_spec.rb
@@ -389,7 +389,10 @@ describe Gitlab::Git::Repository do # rubocop:disable Metrics/BlockLength
     let(:branch_head) { '6d394385cf567f80a8fd85055db1ab4c5295806f' }
     let(:source_sha) { 'cfe32cf61b73a0d5e9f13e774abde7ff789b1660' }
     let(:branch) { 'test-master' }
+    let(:first_parent_ref) { 'refs/heads/test-master' }
     let(:target_ref) { 'refs/merge-requests/999/merge' }
+    let(:arg_branch) {}
+    let(:arg_first_parent_ref) { first_parent_ref }
 
     before do
       create_branch(repository, branch, branch_head)
@@ -399,20 +402,31 @@ describe Gitlab::Git::Repository do # rubocop:disable Metrics/BlockLength
       repository.rugged.references[target_ref]
     end
 
-    it 'changes target ref to a merge between source SHA and branch' do
-      expect(fetch_target_ref).to be_nil
+    shared_examples_for 'correct behavior' do
+      it 'changes target ref to a merge between source SHA and branch' do
+        expect(fetch_target_ref).to be_nil
 
-      merge_commit_id = repository.merge_to_ref(user, source_sha, branch, target_ref, 'foo')
+        merge_commit_id = repository.merge_to_ref(user, source_sha, arg_branch, target_ref, 'foo', arg_first_parent_ref)
 
-      ref = fetch_target_ref
+        ref = fetch_target_ref
 
-      expect(ref.target.oid).to eq(merge_commit_id)
+        expect(ref.target.oid).to eq(merge_commit_id)
+      end
+
+      it 'does not change the branch HEAD' do
+        expect { repository.merge_to_ref(user, source_sha, arg_branch, target_ref, 'foo', arg_first_parent_ref) }
+          .not_to change { repository.find_ref(first_parent_ref).target }
+          .from(branch_head)
+      end
     end
 
-    it 'does not change the branch HEAD' do
-      expect { repository.merge_to_ref(user, source_sha, branch, target_ref, 'foo') }
-        .not_to change { repository.find_branch(branch).target }
-        .from(branch_head)
+    it_behaves_like 'correct behavior'
+
+    context 'when legacy branch parameter is specified and ref path is empty' do
+      it_behaves_like 'correct behavior' do
+        let(:arg_branch) { branch }
+        let(:arg_first_parent_ref) {}
+      end
     end
   end