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 0000000000000000000000000000000000000000..8aaa4a7fc5dea9435da72721851c97c6308777b0 --- /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 ef83ae147d8c679d39cfc89d00aa17685b5d381f..f1e0b072e0bb09d9312640ff73d29b72991c0198 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 3fd2dc6764a082f61b28b779faed53c4a6b7d8eb..3e59959dfbf525fbf3d53f955078781b7c0b8d22 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 dd44a76bec90041b335216ca2306b91dd17681dd..15ae06677bb849c17fcd1197fea38da1d34fd344 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 b9a0be0043377dfb36f327207b4d9c34c94939f7..2492fe8ec336fec98d181da7bb8fa246b4700a0f 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 a4a0355e92601d8d833cdf8ad033d611ca6bdb34..dd8092af82d7b69c2120efab28439c7b0ef39e46 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 1ad7f4c777d443ea436727b187b480961e8c2065..1a6eb99b678d20ada8e8c10d085432d899c26d36 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 852b3f332a345005bb00b23737a83db055e81330..604b48429036106d4d69e35376e96deb39a5fdb8 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 4243b77c7d3b236ddde9b73c38a4f2062813a01e..89655fb3fc0b5f72f352368f42ad2e5fb1da60c3 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 6530ef342d29e4be37d31a443a2dbf381cd0b9fe..f011f1aaadedca1d96d30a7b92b1b369ddea15e6 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 b7b1797abb6649135355042c3b22da8e78e8bd87..66b5450840162ffe5f7d9b1952f5ffc7f0a0aef0 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 6c056efc8cd65106c091d415d6ff54ba55dd755f..672c9536136a0d476db8d1aa14f6283ec205b560 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