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, '') + shared_examples 'creating 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 "should create a new branch" do + expect(repository.create_branch('new_branch', 'master')).not_to be_nil + end + + 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 create a new branch" do - expect(@repo.create_branch('new_branch', 'master')).not_to be_nil + context 'when Gitaly create_branch feature is enabled' do + it_behaves_like 'creating a branch' 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 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") - 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") - 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')