From a97ff8aae09054f1394da18097bacb5b10a5d809 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Wed, 20 Sep 2017 19:28:57 -0300 Subject: Add a factory for `Gitaly::GitCommit`s --- spec/factories/gitaly/commit.rb | 17 +++++++++++++++++ spec/factories/gitaly/commit_author.rb | 9 +++++++++ spec/lib/gitlab/git/commit_spec.rb | 32 +++++--------------------------- 3 files changed, 31 insertions(+), 27 deletions(-) create mode 100644 spec/factories/gitaly/commit.rb create mode 100644 spec/factories/gitaly/commit_author.rb diff --git a/spec/factories/gitaly/commit.rb b/spec/factories/gitaly/commit.rb new file mode 100644 index 00000000000..e7966cee77b --- /dev/null +++ b/spec/factories/gitaly/commit.rb @@ -0,0 +1,17 @@ +FactoryGirl.define do + sequence(:gitaly_commit_id) { Digest::SHA1.hexdigest(Time.now.to_f.to_s) } + + factory :gitaly_commit, class: Gitaly::GitCommit do + skip_create + + id { generate(:gitaly_commit_id) } + parent_ids do + ids = [generate(:gitaly_commit_id), generate(:gitaly_commit_id)] + Google::Protobuf::RepeatedField.new(:string, ids) + end + subject { "My commit" } + body { subject + "\nMy body" } + author { build(:gitaly_commit_author) } + committer { build(:gitaly_commit_author) } + end +end diff --git a/spec/factories/gitaly/commit_author.rb b/spec/factories/gitaly/commit_author.rb new file mode 100644 index 00000000000..341873a2002 --- /dev/null +++ b/spec/factories/gitaly/commit_author.rb @@ -0,0 +1,9 @@ +FactoryGirl.define do + factory :gitaly_commit_author, class: Gitaly::CommitAuthor do + skip_create + + name { generate(:name) } + email { generate(:email) } + date { Google::Protobuf::Timestamp.new(seconds: Time.now.to_i) } + end +end diff --git a/spec/lib/gitlab/git/commit_spec.rb b/spec/lib/gitlab/git/commit_spec.rb index a3dff6d0d4b..3815055139a 100644 --- a/spec/lib/gitlab/git/commit_spec.rb +++ b/spec/lib/gitlab/git/commit_spec.rb @@ -65,34 +65,12 @@ describe Gitlab::Git::Commit, seed_helper: true do end describe "Commit info from gitaly commit" do - let(:id) { 'f00' } - let(:parent_ids) { %w(b45 b46) } let(:subject) { "My commit".force_encoding('ASCII-8BIT') } let(:body) { subject + "My body".force_encoding('ASCII-8BIT') } - let(:committer) do - Gitaly::CommitAuthor.new( - name: generate(:name), - email: generate(:email), - date: Google::Protobuf::Timestamp.new(seconds: 123) - ) - end - let(:author) do - Gitaly::CommitAuthor.new( - name: generate(:name), - email: generate(:email), - date: Google::Protobuf::Timestamp.new(seconds: 456) - ) - end - let(:gitaly_commit) do - Gitaly::GitCommit.new( - id: id, - subject: subject, - body: body, - author: author, - committer: committer, - parent_ids: parent_ids - ) - end + let(:gitaly_commit) { build(:gitaly_commit, subject: subject, body: body) } + let(:id) { gitaly_commit.id } + let(:committer) { gitaly_commit.committer } + let(:author) { gitaly_commit.author } let(:commit) { described_class.new(repository, gitaly_commit) } it { expect(commit.short_id).to eq(id[0..10]) } @@ -104,7 +82,7 @@ describe Gitlab::Git::Commit, seed_helper: true do it { expect(commit.author_name).to eq(author.name) } it { expect(commit.committer_name).to eq(committer.name) } it { expect(commit.committer_email).to eq(committer.email) } - it { expect(commit.parent_ids).to eq(parent_ids) } + it { expect(commit.parent_ids).to eq(gitaly_commit.parent_ids) } context 'no body' do let(:body) { "".force_encoding('ASCII-8BIT') } -- cgit v1.2.1 From 16f850033fa557d86e5f561001f5f67e17a132bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Wed, 20 Sep 2017 19:33:56 -0300 Subject: Populate `Gitlay::Repository`'s `gl_repository` field --- lib/gitlab/git/repository.rb | 2 +- lib/gitlab/gitaly_client/util.rb | 3 ++- spec/lib/gitlab/gitaly_client/util_spec.rb | 43 ++++++++++++++++++++++++++++++ spec/lib/gitlab/workhorse_spec.rb | 3 ++- 4 files changed, 48 insertions(+), 3 deletions(-) create mode 100644 spec/lib/gitlab/gitaly_client/util_spec.rb diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index ef76245a608..9ea5ed04b13 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -1062,7 +1062,7 @@ module Gitlab end def gitaly_repository - Gitlab::GitalyClient::Util.repository(@storage, @relative_path) + Gitlab::GitalyClient::Util.repository(@storage, @relative_path, @gl_repository) end def gitaly_operations_client diff --git a/lib/gitlab/gitaly_client/util.rb b/lib/gitlab/gitaly_client/util.rb index 2fb5875a7a2..da43c616b94 100644 --- a/lib/gitlab/gitaly_client/util.rb +++ b/lib/gitlab/gitaly_client/util.rb @@ -2,10 +2,11 @@ module Gitlab module GitalyClient module Util class << self - def repository(repository_storage, relative_path) + def repository(repository_storage, relative_path, gl_repository) Gitaly::Repository.new( storage_name: repository_storage, relative_path: relative_path, + gl_repository: gl_repository, git_object_directory: Gitlab::Git::Env['GIT_OBJECT_DIRECTORY'].to_s, git_alternate_object_directories: Array.wrap(Gitlab::Git::Env['GIT_ALTERNATE_OBJECT_DIRECTORIES']) ) diff --git a/spec/lib/gitlab/gitaly_client/util_spec.rb b/spec/lib/gitlab/gitaly_client/util_spec.rb new file mode 100644 index 00000000000..498f6886bee --- /dev/null +++ b/spec/lib/gitlab/gitaly_client/util_spec.rb @@ -0,0 +1,43 @@ +require 'spec_helper' + +describe Gitlab::GitalyClient::Util do + describe '.repository' do + let(:repository_storage) { 'default' } + let(:relative_path) { 'my/repo.git' } + let(:gl_repository) { 'project-1' } + let(:git_object_directory) { '.git/objects' } + let(:git_alternate_object_directory) { '/dir/one:/dir/two' } + + subject do + described_class.repository(repository_storage, relative_path, gl_repository) + end + + it 'creates a Gitaly::Repository with the given data' do + expect(Gitlab::Git::Env).to receive(:[]).with('GIT_OBJECT_DIRECTORY') + .and_return(git_object_directory) + expect(Gitlab::Git::Env).to receive(:[]).with('GIT_ALTERNATE_OBJECT_DIRECTORIES') + .and_return(git_alternate_object_directory) + + expect(subject).to be_a(Gitaly::Repository) + expect(subject.storage_name).to eq(repository_storage) + expect(subject.relative_path).to eq(relative_path) + expect(subject.gl_repository).to eq(gl_repository) + expect(subject.git_object_directory).to eq(git_object_directory) + expect(subject.git_alternate_object_directories).to eq([git_alternate_object_directory]) + end + end + + describe '.gitaly_user' do + let(:user) { create(:user) } + let(:gl_id) { Gitlab::GlId.gl_id(user) } + + subject { described_class.gitaly_user(user) } + + it 'creates a Gitaly::User from a GitLab user' do + expect(subject).to be_a(Gitaly::User) + expect(subject.name).to eq(user.name) + expect(subject.email).to eq(user.email) + expect(subject.gl_id).to eq(gl_id) + end + end +end diff --git a/spec/lib/gitlab/workhorse_spec.rb b/spec/lib/gitlab/workhorse_spec.rb index 5708aa6754f..5f4d69224eb 100644 --- a/spec/lib/gitlab/workhorse_spec.rb +++ b/spec/lib/gitlab/workhorse_spec.rb @@ -216,7 +216,8 @@ describe Gitlab::Workhorse do it 'includes a Repository param' do repo_param = { storage_name: 'default', - relative_path: project.full_path + '.git' + relative_path: project.full_path + '.git', + gl_repository: "project-#{project.id}" } expect(subject[:Repository]).to include(repo_param) -- cgit v1.2.1 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: Implement OperationService.UserAddBranch Gitaly RPC --- lib/gitlab/git/repository.rb | 34 ++++++++-- lib/gitlab/gitaly_client/operation_service.rb | 20 ++++++ .../gitlab/gitaly_client/operation_service_spec.rb | 55 ++++++++++++++++ spec/models/repository_spec.rb | 75 ++++++++++++++-------- 4 files changed, 152 insertions(+), 32 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 - end + subject { repository.add_branch(user, branch_name, target) } - it 'creates the branch' do - allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([true, nil]) + 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) - branch = repository.add_branch(user, 'new_feature', 'master') + subject + end - 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 -- cgit v1.2.1