Merge branch 'gitaly-add-branch' into 'master'

Implement OperationService.UserAddBranch Gitaly RPC

Closes gitaly#544

See merge request gitlab-org/gitlab-ce!14522

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)
@@ -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
@@ -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

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

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'])
           )

spec/factories/gitaly/commit.rb

+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

spec/factories/gitaly/commit_author.rb

+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

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') }

spec/lib/gitlab/gitaly_client/operation_service_spec.rb

+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

spec/lib/gitlab/gitaly_client/util_spec.rb

+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

spec/lib/gitlab/workhorse_spec.rb

@@ -226,7 +226,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)

spec/models/repository_spec.rb

@@ -815,26 +815,54 @@ describe Repository do
   end
 
   describe '#add_branch' do
+    let(:branch_name) { 'new_feature' }
+    let(:target) { 'master' }
+
+    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
+        expect(subject.name).to eq(branch_name)
+        expect(repository.find_branch(branch_name)).not_to be_nil
+      end
+
+      context 'with a non-existing target' do
+        let(:target) { 'fake-target' }
+
+        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 '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 { repository.add_branch(user, 'new_feature', 'master') }.not_to raise_error
+          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])
 
-        branch = repository.add_branch(user, 'new_feature', 'master')
-
-        expect(branch.name).to eq('new_feature')
+          expect(subject.name).to eq(branch_name)
         end
 
         it 'calls the after_create_branch hook' do
           expect(repository).to receive(:after_create_branch)
 
-        repository.add_branch(user, 'new_feature', 'master')
+          subject
         end
       end
 
@@ -842,18 +870,15 @@ describe Repository 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 { 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 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)
+          expect(repository.find_branch(branch_name)).to be_nil
+        end
       end
     end
   end