Merge branch 'feature/migrate-branch-operations-to-gitaly' into 'master'37407-error-500-when-creating-a-project-createservice-returns-a-nil-object

Migrate creating/deleting a branch to Gitaly

See merge request !13864

6 files changed, 124 insertions, 44 deletions

diff --git a/lib/github/representation/branch.rb b/lib/github/representation/branch.rb
index c6fa928d565..823e8e9a9c4 100644
--- a/lib/github/representation/branch.rb
+++ b/lib/github/representation/branch.rb
@@ -41,7 +41,7 @@ module Github
 
       def remove!(name)
         repository.delete_branch(name)
-      rescue Rugged::ReferenceError => e
+      rescue Gitlab::Git::Repository::DeleteBranchError => e
         Rails.logger.error("#{self.class.name}: Could not remove branch #{name}: #{e}")
       end
 
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index 75d4efc0bc5..efa13590a2c 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -18,6 +18,7 @@ module Gitlab
       InvalidBlobName = Class.new(StandardError)
       InvalidRef = Class.new(StandardError)
       GitError = Class.new(StandardError)
+      DeleteBranchError = Class.new(StandardError)
 
       class << self
         # Unlike `new`, `create` takes the storage path, not the storage name
@@ -653,10 +654,16 @@ module Gitlab
       end
 
       # Delete the specified branch from the repository
-      #
-      # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/476
       def delete_branch(branch_name)
-        rugged.branches.delete(branch_name)
+        gitaly_migrate(:delete_branch) do |is_enabled|
+          if is_enabled
+            gitaly_ref_client.delete_branch(branch_name)
+          else
+            rugged.branches.delete(branch_name)
+          end
+        end
+      rescue Rugged::ReferenceError, CommandError => e
+        raise DeleteBranchError, e
       end
 
       def delete_refs(*ref_names)
@@ -681,15 +688,14 @@ module Gitlab
       # Examples:
       #   create_branch("feature")
       #   create_branch("other-feature", "master")
-      #
-      # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/476
       def create_branch(ref, start_point = "HEAD")
-        rugged_ref = rugged.branches.create(ref, start_point)
-        target_commit = Gitlab::Git::Commit.find(self, rugged_ref.target)
-        Gitlab::Git::Branch.new(self, rugged_ref.name, rugged_ref.target, target_commit)
-      rescue Rugged::ReferenceError => e
-        raise InvalidRef.new("Branch #{ref} already exists") if e.to_s =~ /'refs\/heads\/#{ref}'/
-        raise InvalidRef.new("Invalid reference #{start_point}")
+        gitaly_migrate(:create_branch) do |is_enabled|
+          if is_enabled
+            gitaly_ref_client.create_branch(ref, start_point)
+          else
+            rugged_create_branch(ref, start_point)
+          end
+        end
       end
 
       # Delete the specified remote from this repository.
@@ -1226,6 +1232,15 @@ module Gitlab
         false
       end
 
+      def rugged_create_branch(ref, start_point)
+        rugged_ref = rugged.branches.create(ref, start_point)
+        target_commit = Gitlab::Git::Commit.find(self, rugged_ref.target)
+        Gitlab::Git::Branch.new(self, rugged_ref.name, rugged_ref.target, target_commit)
+      rescue Rugged::ReferenceError => e
+        raise InvalidRef.new("Branch #{ref} already exists") if e.to_s =~ /'refs\/heads\/#{ref}'/
+        raise InvalidRef.new("Invalid reference #{start_point}")
+      end
+
       def gitaly_copy_gitattributes(revision)
         gitaly_repository_client.apply_gitattributes(revision)
       end
diff --git a/lib/gitlab/gitaly_client/ref_service.rb b/lib/gitlab/gitaly_client/ref_service.rb
index a1a25cf2079..8ef873d5848 100644
--- a/lib/gitlab/gitaly_client/ref_service.rb
+++ b/lib/gitlab/gitaly_client/ref_service.rb
@@ -79,7 +79,7 @@ module Gitlab
       end
 
       def find_branch(branch_name)
-        request = Gitaly::DeleteBranchRequest.new(
+        request = Gitaly::FindBranchRequest.new(
           repository: @gitaly_repo,
           name: GitalyClient.encode(branch_name)
         )
@@ -92,6 +92,40 @@ module Gitlab
         Gitlab::Git::Branch.new(@repository, encode!(branch.name.dup), branch.target_commit.id, target_commit)
       end
 
+      def create_branch(ref, start_point)
+        request = Gitaly::CreateBranchRequest.new(
+          repository: @gitaly_repo,
+          name: GitalyClient.encode(ref),
+          start_point: GitalyClient.encode(start_point)
+        )
+
+        response = GitalyClient.call(@repository.storage, :ref_service, :create_branch, request)
+
+        case response.status
+        when :OK
+          branch = response.branch
+          target_commit = Gitlab::Git::Commit.decorate(@repository, branch.target_commit)
+          Gitlab::Git::Branch.new(@repository, branch.name, branch.target_commit.id, target_commit)
+        when :ERR_INVALID
+          invalid_ref!("Invalid ref name")
+        when :ERR_EXISTS
+          invalid_ref!("Branch #{ref} already exists")
+        when :ERR_INVALID_START_POINT
+          invalid_ref!("Invalid reference #{start_point}")
+        else
+          raise "Unknown response status: #{response.status}"
+        end
+      end
+
+      def delete_branch(branch_name)
+        request = Gitaly::DeleteBranchRequest.new(
+          repository: @gitaly_repo,
+          name: GitalyClient.encode(branch_name)
+        )
+
+        GitalyClient.call(@repository.storage, :ref_service, :delete_branch, request)
+      end
+
       private
 
       def consume_refs_response(response)
@@ -163,6 +197,10 @@ module Gitlab
 
         Gitlab::Git::Commit.decorate(@repository, hash)
       end
+
+      def invalid_ref!(message)
+        raise Gitlab::Git::Repository::InvalidRef.new(message)
+      end
     end
   end
 end
diff --git a/lib/gitlab/github_import/importer.rb b/lib/gitlab/github_import/importer.rb
index 373062b354b..b8c07460ebb 100644
--- a/lib/gitlab/github_import/importer.rb
+++ b/lib/gitlab/github_import/importer.rb
@@ -166,7 +166,7 @@ module Gitlab
 
       def remove_branch(name)
         project.repository.delete_branch(name)
-      rescue Rugged::ReferenceError
+      rescue Gitlab::Git::Repository::DeleteBranchFailed
         errors << { type: :remove_branch, name: name }
       end
 
diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb
index 08959e7bc16..556a148c3bc 100644
--- a/spec/lib/gitlab/git/repository_spec.rb
+++ b/spec/lib/gitlab/git/repository_spec.rb
@@ -390,46 +390,73 @@ describe Gitlab::Git::Repository, seed_helper: true do
   end
 
   describe "#delete_branch" do
-    before(:all) do
-      @repo = Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '')
-      @repo.delete_branch("feature")
+    shared_examples "deleting a branch" do
+      let(:repository) { Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '') }
+
+      after do
+        FileUtils.rm_rf(TEST_MUTABLE_REPO_PATH)
+        ensure_seeds
+      end
+
+      it "removes the branch from the repo" do
+        branch_name = "to-be-deleted-soon"
+
+        repository.create_branch(branch_name)
+        expect(repository.rugged.branches[branch_name]).not_to be_nil
+
+        repository.delete_branch(branch_name)
+        expect(repository.rugged.branches[branch_name]).to be_nil
+      end
+
+      context "when branch does not exist" do
+        it "raises a DeleteBranchError exception" do
+          expect { repository.delete_branch("this-branch-does-not-exist") }.to raise_error(Gitlab::Git::Repository::DeleteBranchError)
+        end
+      end
     end
 
-    it "should remove the branch from the repo" do
-      expect(@repo.rugged.branches["feature"]).to be_nil
+    context "when Gitaly delete_branch is enabled" do
+      it_behaves_like "deleting a branch"
     end
 
-    after(:all) do
-      FileUtils.rm_rf(TEST_MUTABLE_REPO_PATH)
-      ensure_seeds
+    context "when Gitaly delete_branch is disabled", skip_gitaly_mock: true do
+      it_behaves_like "deleting a branch"
     end
   end
 
   describe "#create_branch" do
-    before(:all) do
-      @repo = Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '')
-    end
+    shared_examples 'creating a branch' do
+      let(:repository) { Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '') }
 
-    it "should create a new branch" do
-      expect(@repo.create_branch('new_branch', 'master')).not_to be_nil
-    end
+      after do
+        FileUtils.rm_rf(TEST_MUTABLE_REPO_PATH)
+        ensure_seeds
+      end
 
-    it "should create a new branch with the right name" do
-      expect(@repo.create_branch('another_branch', 'master').name).to eq('another_branch')
-    end
+      it "should create a new branch" do
+        expect(repository.create_branch('new_branch', 'master')).not_to be_nil
+      end
 
-    it "should fail if we create an existing branch" do
-      @repo.create_branch('duplicated_branch', 'master')
-      expect {@repo.create_branch('duplicated_branch', 'master')}.to raise_error("Branch duplicated_branch already exists")
+      it "should create a new branch with the right name" do
+        expect(repository.create_branch('another_branch', 'master').name).to eq('another_branch')
+      end
+
+      it "should fail if we create an existing branch" do
+        repository.create_branch('duplicated_branch', 'master')
+        expect {repository.create_branch('duplicated_branch', 'master')}.to raise_error("Branch duplicated_branch already exists")
+      end
+
+      it "should fail if we create a branch from a non existing ref" do
+        expect {repository.create_branch('branch_based_in_wrong_ref', 'master_2_the_revenge')}.to raise_error("Invalid reference master_2_the_revenge")
+      end
     end
 
-    it "should fail if we create a branch from a non existing ref" do
-      expect {@repo.create_branch('branch_based_in_wrong_ref', 'master_2_the_revenge')}.to raise_error("Invalid reference master_2_the_revenge")
+    context 'when Gitaly create_branch feature is enabled' do
+      it_behaves_like 'creating a branch'
     end
 
-    after(:all) do
-      FileUtils.rm_rf(TEST_MUTABLE_REPO_PATH)
-      ensure_seeds
+    context 'when Gitaly create_branch feature is disabled', skip_gitaly_mock: true do
+      it_behaves_like 'creating a branch'
     end
   end
 
@@ -905,7 +932,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
     it 'should set the autocrlf option to the provided option' do
       @repo.autocrlf = :input
 
-      File.open(File.join(SEED_STORAGE_PATH, TEST_MUTABLE_REPO_PATH, '.git', 'config')) do |config_file|
+      File.open(File.join(SEED_STORAGE_PATH, TEST_MUTABLE_REPO_PATH, 'config')) do |config_file|
         expect(config_file.read).to match('autocrlf = input')
       end
     end
@@ -977,7 +1004,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
 
     context 'with local and remote branches' do
       let(:repository) do
-        Gitlab::Git::Repository.new('default', File.join(TEST_MUTABLE_REPO_PATH, '.git'), '')
+        Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '')
       end
 
       before do
@@ -1024,7 +1051,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
 
     context 'with local and remote branches' do
       let(:repository) do
-        Gitlab::Git::Repository.new('default', File.join(TEST_MUTABLE_REPO_PATH, '.git'), '')
+        Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '')
       end
 
       before do
@@ -1230,7 +1257,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
 
   describe '#local_branches' do
     before(:all) do
-      @repo = Gitlab::Git::Repository.new('default', File.join(TEST_MUTABLE_REPO_PATH, '.git'), '')
+      @repo = Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '')
     end
 
     after(:all) do
diff --git a/spec/support/seed_helper.rb b/spec/support/seed_helper.rb
index 8731847592b..11ef1fc477f 100644
--- a/spec/support/seed_helper.rb
+++ b/spec/support/seed_helper.rb
@@ -41,7 +41,7 @@ module SeedHelper
   end
 
   def create_mutable_seeds
-    system(git_env, *%W(#{Gitlab.config.git.bin_path} clone #{TEST_REPO_PATH} #{TEST_MUTABLE_REPO_PATH}),
+    system(git_env, *%W(#{Gitlab.config.git.bin_path} clone --bare #{TEST_REPO_PATH} #{TEST_MUTABLE_REPO_PATH}),
            chdir: SEED_STORAGE_PATH,
            out: '/dev/null',
            err: '/dev/null')