From 3f0e6d92055e7e1923a51684a397a2a150475899 Mon Sep 17 00:00:00 2001 From: "Jacob Vosmaer (GitLab)" Date: Fri, 27 Jul 2018 08:43:19 +0000 Subject: [PATCH] More Gitaly cleanup: fetch_ref, allow disk access blocks --- lib/gitlab/git/conflict/resolver.rb | 2 +- lib/gitlab/git/repository.rb | 30 ++++--------------- lib/gitlab/git/repository_mirroring.rb | 4 ++- .../import_export/merge_request_parser.rb | 6 +++- .../project_tree_restorer_spec.rb | 2 +- spec/models/repository_spec.rb | 6 +--- 6 files changed, 17 insertions(+), 33 deletions(-) diff --git a/lib/gitlab/git/conflict/resolver.rb b/lib/gitlab/git/conflict/resolver.rb index 0e4a973301f..6dc792c16b8 100644 --- a/lib/gitlab/git/conflict/resolver.rb +++ b/lib/gitlab/git/conflict/resolver.rb @@ -17,7 +17,7 @@ module Gitlab end rescue GRPC::FailedPrecondition => e raise Gitlab::Git::Conflict::Resolver::ConflictSideMissing.new(e.message) - rescue Rugged::ReferenceError, Rugged::OdbError, GRPC::BadStatus => e + rescue GRPC::BadStatus => e raise Gitlab::Git::CommandError.new(e) end diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 0356e8efc5c..eb02c7ac8e1 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -558,7 +558,9 @@ module Gitlab if is_enabled gitaly_operation_client.user_update_branch(branch_name, user, newrev, oldrev) else - OperationService.new(user, self).update_branch(branch_name, newrev, oldrev) + Gitlab::GitalyClient::StorageSettings.allow_disk_access do + OperationService.new(user, self).update_branch(branch_name, newrev, oldrev) + end end end end @@ -832,18 +834,9 @@ module Gitlab Gitlab::Git.check_namespace!(source_repository) source_repository = RemoteRepository.new(source_repository) unless source_repository.is_a?(RemoteRepository) - message, status = GitalyClient.migrate(:fetch_ref) do |is_enabled| - if is_enabled - gitaly_fetch_ref(source_repository, source_ref: source_ref, target_ref: target_ref) - else - # When removing this code, also remove source_repository#path - # to remove deprecated method calls - local_fetch_ref(source_repository.path, source_ref: source_ref, target_ref: target_ref) - end - end - - # Make sure ref was created, and raise Rugged::ReferenceError when not - raise Rugged::ReferenceError, message if status != 0 + args = %W(fetch --no-tags -f #{GITALY_INTERNAL_URL} #{source_ref}:#{target_ref}) + message, status = run_git(args, env: source_repository.fetch_env) + raise Gitlab::Git::CommandError, message if status != 0 target_ref end @@ -1244,17 +1237,6 @@ module Gitlab gitaly_repository_client.apply_gitattributes(revision) end - def local_fetch_ref(source_path, source_ref:, target_ref:) - args = %W(fetch --no-tags -f #{source_path} #{source_ref}:#{target_ref}) - run_git(args) - end - - def gitaly_fetch_ref(source_repository, source_ref:, target_ref:) - args = %W(fetch --no-tags -f #{GITALY_INTERNAL_URL} #{source_ref}:#{target_ref}) - - run_git(args, env: source_repository.fetch_env) - end - def gitaly_delete_refs(*ref_names) gitaly_ref_client.delete_refs(refs: ref_names) if ref_names.any? end diff --git a/lib/gitlab/git/repository_mirroring.rb b/lib/gitlab/git/repository_mirroring.rb index 8835bfb2481..65eb5cc18cf 100644 --- a/lib/gitlab/git/repository_mirroring.rb +++ b/lib/gitlab/git/repository_mirroring.rb @@ -6,7 +6,9 @@ module Gitlab if is_enabled gitaly_ref_client.remote_branches(remote_name) else - rugged_remote_branches(remote_name) + Gitlab::GitalyClient::StorageSettings.allow_disk_access do + rugged_remote_branches(remote_name) + end end end end diff --git a/lib/gitlab/import_export/merge_request_parser.rb b/lib/gitlab/import_export/merge_request_parser.rb index d0527f014a7..62a1833b39c 100644 --- a/lib/gitlab/import_export/merge_request_parser.rb +++ b/lib/gitlab/import_export/merge_request_parser.rb @@ -27,7 +27,11 @@ module Gitlab # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/1295 def fetch_ref - @project.repository.fetch_ref(@project.repository, source_ref: @diff_head_sha, target_ref: @merge_request.source_branch) + target_ref = Gitlab::Git::BRANCH_REF_PREFIX + @merge_request.source_branch + + unless @project.repository.fetch_source_branch!(@project.repository, @diff_head_sha, target_ref) + Rails.logger.warn("Import/Export warning: Failed to create #{target_ref} for MR: #{@merge_request.iid}") + end end def branch_exists?(branch_name) diff --git a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb index bac5693c830..a88ac0a091e 100644 --- a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb +++ b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb @@ -16,7 +16,7 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do @shared = @project.import_export_shared allow(@shared).to receive(:export_path).and_return('spec/lib/gitlab/import_export/') - allow_any_instance_of(Repository).to receive(:fetch_ref).and_return(true) + allow_any_instance_of(Repository).to receive(:fetch_source_branch!).and_return(true) allow_any_instance_of(Gitlab::Git::Repository).to receive(:branch_exists?).and_return(false) expect_any_instance_of(Gitlab::Git::Repository).to receive(:create_branch).with('feature', 'DCBA') diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 5d64602ca56..52ec8dbe25a 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -1129,16 +1129,12 @@ describe Repository do end it 'raises Rugged::ReferenceError' do - raise_reference_error = raise_error(Rugged::ReferenceError) do |err| - expect(err.cause).to be_nil - end - expect do Gitlab::Git::OperationService.new(git_user, target_project.repository.raw_repository) .with_branch('feature', start_repository: project.repository.raw_repository, &:itself) - end.to raise_reference_error + end.to raise_error(Gitlab::Git::CommandError) end end