From fa5f0164eba8a03ba4fa1403849f3996577fd2e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Wed, 20 Sep 2017 19:34:30 -0300 Subject: [PATCH] Implement OperationService.UserAddBranch Gitaly RPC --- lib/gitlab/git/repository.rb | 34 ++++++-- lib/gitlab/gitaly_client/operation_service.rb | 20 +++++ .../gitaly_client/operation_service_spec.rb | 55 +++++++++++++ spec/models/repository_spec.rb | 77 ++++++++++++------- 4 files changed, 153 insertions(+), 33 deletions(-) create mode 100644 spec/lib/gitlab/gitaly_client/operation_service_spec.rb diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 9ea5ed04b13..22b735c6f7b 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -656,13 +656,13 @@ module Gitlab end def add_branch(branch_name, user:, target:) - target_object = Ref.dereference_object(lookup(target)) - raise InvalidRef.new("target not found: #{target}") unless target_object - - OperationService.new(user, self).add_branch(branch_name, target_object.oid) - find_branch(branch_name) - rescue Rugged::ReferenceError => ex - raise InvalidRef, ex + gitaly_migrate(:operation_user_create_branch) do |is_enabled| + if is_enabled + gitaly_add_branch(branch_name, user, target) + else + rugged_add_branch(branch_name, user, target) + end + end end def add_tag(tag_name, user:, target:, message: nil) @@ -1081,6 +1081,10 @@ module Gitlab @gitaly_repository_client ||= Gitlab::GitalyClient::RepositoryService.new(self) end + def gitaly_operation_client + @gitaly_operation_client ||= Gitlab::GitalyClient::OperationService.new(self) + end + def gitaly_migrate(method, status: Gitlab::GitalyClient::MigrationStatus::OPT_IN, &block) Gitlab::GitalyClient.migrate(method, status: status, &block) rescue GRPC::NotFound => e @@ -1472,6 +1476,22 @@ module Gitlab file.write(gitattributes_content) end end + + def gitaly_add_branch(branch_name, user, target) + gitaly_operation_client.user_create_branch(branch_name, user, target) + rescue GRPC::FailedPrecondition => ex + raise InvalidRef, ex + end + + def rugged_add_branch(branch_name, user, target) + target_object = Ref.dereference_object(lookup(target)) + raise InvalidRef.new("target not found: #{target}") unless target_object + + OperationService.new(user, self).add_branch(branch_name, target_object.oid) + find_branch(branch_name) + rescue Rugged::ReferenceError + raise InvalidRef, ex + end end end end diff --git a/lib/gitlab/gitaly_client/operation_service.rb b/lib/gitlab/gitaly_client/operation_service.rb index 2d5440e7ea8..46bd5c18603 100644 --- a/lib/gitlab/gitaly_client/operation_service.rb +++ b/lib/gitlab/gitaly_client/operation_service.rb @@ -40,6 +40,26 @@ module Gitlab rescue GRPC::FailedPrecondition => e raise Gitlab::Git::Repository::InvalidRef, e end + + def user_create_branch(branch_name, user, start_point) + request = Gitaly::UserCreateBranchRequest.new( + repository: @gitaly_repo, + branch_name: GitalyClient.encode(branch_name), + user: Util.gitaly_user(user), + start_point: GitalyClient.encode(start_point) + ) + response = GitalyClient.call(@repository.storage, :operation_service, + :user_create_branch, request) + if response.pre_receive_error.present? + raise Gitlab::Git::HooksService::PreReceiveError.new(response.pre_receive_error) + end + + branch = response.branch + return nil unless branch + + target_commit = Gitlab::Git::Commit.decorate(@repository, branch.target_commit) + Gitlab::Git::Branch.new(@repository, branch.name, target_commit.id, target_commit) + end end end end diff --git a/spec/lib/gitlab/gitaly_client/operation_service_spec.rb b/spec/lib/gitlab/gitaly_client/operation_service_spec.rb new file mode 100644 index 00000000000..769b14687ac --- /dev/null +++ b/spec/lib/gitlab/gitaly_client/operation_service_spec.rb @@ -0,0 +1,55 @@ +require 'spec_helper' + +describe Gitlab::GitalyClient::OperationService do + let(:project) { create(:project) } + let(:repository) { project.repository.raw } + let(:client) { described_class.new(repository) } + + describe '#user_create_branch' do + let(:user) { create(:user) } + let(:gitaly_user) { Gitlab::GitalyClient::Util.gitaly_user(user) } + let(:branch_name) { 'new' } + let(:start_point) { 'master' } + let(:request) do + Gitaly::UserCreateBranchRequest.new( + repository: repository.gitaly_repository, + branch_name: branch_name, + start_point: start_point, + user: gitaly_user + ) + end + let(:gitaly_commit) { build(:gitaly_commit) } + let(:commit_id) { gitaly_commit.id } + let(:gitaly_branch) do + Gitaly::Branch.new(name: branch_name, target_commit: gitaly_commit) + end + let(:response) { Gitaly::UserCreateBranchResponse.new(branch: gitaly_branch) } + let(:commit) { Gitlab::Git::Commit.new(repository, gitaly_commit) } + + subject { client.user_create_branch(branch_name, user, start_point) } + + it 'sends a user_create_branch message and returns a Gitlab::git::Branch' do + expect_any_instance_of(Gitaly::OperationService::Stub) + .to receive(:user_create_branch).with(request, kind_of(Hash)) + .and_return(response) + + expect(subject.name).to eq(branch_name) + expect(subject.dereferenced_target).to eq(commit) + end + + context "when pre_receive_error is present" do + let(:response) do + Gitaly::UserCreateBranchResponse.new(pre_receive_error: "something failed") + end + + it "throws a PreReceive exception" do + expect_any_instance_of(Gitaly::OperationService::Stub) + .to receive(:user_create_branch).with(request, kind_of(Hash)) + .and_return(response) + + expect { subject }.to raise_error( + Gitlab::Git::HooksService::PreReceiveError, "something failed") + end + end + end +end diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index ab81d39691b..7b562583f37 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -815,45 +815,70 @@ describe Repository do end describe '#add_branch' do - context 'when pre hooks were successful' do - it 'runs without errors' do - hook = double(trigger: [true, nil]) - expect(Gitlab::Git::Hook).to receive(:new).exactly(3).times.and_return(hook) + let(:branch_name) { 'new_feature' } + let(:target) { 'master' } - expect { repository.add_branch(user, 'new_feature', 'master') }.not_to raise_error + subject { repository.add_branch(user, branch_name, target) } + + context 'with Gitaly enabled' do + it "calls Gitaly's OperationService" do + expect_any_instance_of(Gitlab::GitalyClient::OperationService) + .to receive(:user_create_branch).with(branch_name, user, target) + .and_return(nil) + + subject end - it 'creates the branch' do - allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([true, nil]) - - branch = repository.add_branch(user, 'new_feature', 'master') - - expect(branch.name).to eq('new_feature') + it 'creates_the_branch' do + expect(subject.name).to eq(branch_name) + expect(repository.find_branch(branch_name)).not_to be_nil end - it 'calls the after_create_branch hook' do - expect(repository).to receive(:after_create_branch) + context 'with a non-existing target' do + let(:target) { 'fake-target' } - repository.add_branch(user, 'new_feature', 'master') + it "returns false and doesn't create the branch" do + expect(subject).to be(false) + expect(repository.find_branch(branch_name)).to be_nil + end end end - context 'when pre hooks failed' do - it 'gets an error' do - allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([false, '']) + context 'with Gitaly disabled', skip_gitaly_mock: true do + context 'when pre hooks were successful' do + it 'runs without errors' do + hook = double(trigger: [true, nil]) + expect(Gitlab::Git::Hook).to receive(:new).exactly(3).times.and_return(hook) - expect do - repository.add_branch(user, 'new_feature', 'master') - end.to raise_error(Gitlab::Git::HooksService::PreReceiveError) + expect { subject }.not_to raise_error + end + + it 'creates the branch' do + allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([true, nil]) + + expect(subject.name).to eq(branch_name) + end + + it 'calls the after_create_branch hook' do + expect(repository).to receive(:after_create_branch) + + subject + end end - it 'does not create the branch' do - allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([false, '']) + context 'when pre hooks failed' do + it 'gets an error' do + allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([false, '']) - expect do - repository.add_branch(user, 'new_feature', 'master') - end.to raise_error(Gitlab::Git::HooksService::PreReceiveError) - expect(repository.find_branch('new_feature')).to be_nil + expect { subject }.to raise_error(Gitlab::Git::HooksService::PreReceiveError) + end + + it 'does not create the branch' do + allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([false, '']) + + expect { subject }.to raise_error(Gitlab::Git::HooksService::PreReceiveError) + expect(repository.find_branch(branch_name)).to be_nil + end end end end