Remove repository fork code from Gitlab::Shell

The fork_repository call was originally run as a gitlab-shell
subprocess. Now, it's a Gitaly RPC. Removing the indirection makes
this clearer and easier to understand.

Only RepositoryForkWorker calls this RPC, so I haven't added it to the
`Gitlab::Git::Repository` object.

app/workers/repository_fork_worker.rb

@@ -2,7 +2,6 @@
 
 class RepositoryForkWorker # rubocop:disable Scalability/IdempotentWorker
   include ApplicationWorker
-  include Gitlab::ShellAdapter
   include ProjectStartImport
   include ProjectImportOptions
 
@@ -27,18 +26,8 @@ class RepositoryForkWorker # rubocop:disable Scalability/IdempotentWorker
 
     Gitlab::Metrics.add_event(:fork_repository)
 
-    result = gitlab_shell.fork_repository(source_project, target_project)
-
-    if result
-      link_lfs_objects(source_project, target_project)
-    else
-      raise_fork_failure(
-        source_project,
-        target_project,
-        'Failed to create fork repository'
-      )
-    end
-
+    gitaly_fork!(source_project, target_project)
+    link_lfs_objects(source_project, target_project)
     target_project.after_import
   end
 
@@ -49,6 +38,17 @@ class RepositoryForkWorker # rubocop:disable Scalability/IdempotentWorker
     false
   end
 
+  def gitaly_fork!(source_project, target_project)
+    source_repo = source_project.repository.raw
+    target_repo = target_project.repository.raw
+
+    ::Gitlab::GitalyClient::RepositoryService.new(target_repo).fork_repository(source_repo)
+  rescue GRPC::BadStatus => e
+    Gitlab::ErrorTracking.track_exception(e, source_project_id: source_project.id, target_project_id: target_project.id)
+
+    raise_fork_failure(source_project, target_project, 'Failed to create fork repository')
+  end
+
   def link_lfs_objects(source_project, target_project)
     Projects::LfsPointers::LfsLinkService
         .new(target_project)

lib/gitlab/shell.rb

@@ -98,17 +98,6 @@ module Gitlab
       false
     end
 
-    # Fork repository to new path
-    #
-    # @param [Project] source_project forked-from Project
-    # @param [Project] target_project forked-to Project
-    def fork_repository(source_project, target_project)
-      forked_from_relative_path = "#{source_project.disk_path}.git"
-      fork_args = [target_project.repository_storage, "#{target_project.disk_path}.git", target_project.full_path]
-
-      GitalyGitlabProjects.new(source_project.repository_storage, forked_from_relative_path, source_project.full_path).fork_repository(*fork_args)
-    end
-
     # Removes a repository from file system, using rm_diretory which is an alias
     # for rm_namespace. Given the underlying implementation removes the name
     # passed as second argument on the passed storage.
@@ -212,30 +201,5 @@ module Gitlab
       # need to do the same here...
       raise Error, e
     end
-
-    class GitalyGitlabProjects
-      attr_reader :shard_name, :repository_relative_path, :output, :gl_project_path
-
-      def initialize(shard_name, repository_relative_path, gl_project_path)
-        @shard_name = shard_name
-        @repository_relative_path = repository_relative_path
-        @output = ''
-        @gl_project_path = gl_project_path
-      end
-
-      def fork_repository(new_shard_name, new_repository_relative_path, new_project_name)
-        target_repository = Gitlab::Git::Repository.new(new_shard_name, new_repository_relative_path, nil, new_project_name)
-        raw_repository = Gitlab::Git::Repository.new(shard_name, repository_relative_path, nil, gl_project_path)
-
-        Gitlab::GitalyClient::RepositoryService.new(target_repository).fork_repository(raw_repository)
-      rescue GRPC::BadStatus => e
-        logger.error "fork-repository failed: #{e.message}"
-        false
-      end
-
-      def logger
-        Rails.logger # rubocop:disable Gitlab/RailsLogger
-      end
-    end
   end
 end

spec/lib/gitlab/shell_spec.rb

@@ -9,7 +9,6 @@ describe Gitlab::Shell do
   let(:gitlab_shell) { described_class.new }
 
   it { is_expected.to respond_to :remove_repository }
-  it { is_expected.to respond_to :fork_repository }
 
   describe '.url_to_repo' do
     let(:full_path) { 'diaspora/disaspora-rails' }
@@ -95,28 +94,6 @@ describe Gitlab::Shell do
         expect(TestEnv.storage_dir_exists?(project2.repository_storage, "#{project2.disk_path}.git")).to be(true)
       end
     end
-
-    describe '#fork_repository' do
-      let(:target_project) { create(:project) }
-
-      subject do
-        gitlab_shell.fork_repository(project, target_project)
-      end
-
-      it 'returns true when the command succeeds' do
-        expect_any_instance_of(Gitlab::GitalyClient::RepositoryService).to receive(:fork_repository)
-          .with(repository.raw_repository) { :gitaly_response_object }
-
-        is_expected.to be_truthy
-      end
-
-      it 'return false when the command fails' do
-        expect_any_instance_of(Gitlab::GitalyClient::RepositoryService).to receive(:fork_repository)
-          .with(repository.raw_repository) { raise GRPC::BadStatus, 'bla' }
-
-        is_expected.to be_falsy
-      end
-    end
   end
 
   describe 'namespace actions' do

spec/workers/repository_fork_worker_spec.rb

@@ -13,7 +13,6 @@ describe RepositoryForkWorker do
 
   describe "#perform" do
     let(:project) { create(:project, :public, :repository) }
-    let(:shell) { Gitlab::Shell.new }
     let(:forked_project) { create(:project, :repository, :import_scheduled) }
 
     before do
@@ -21,12 +20,17 @@ describe RepositoryForkWorker do
     end
 
     shared_examples 'RepositoryForkWorker performing' do
-      before do
-        allow(subject).to receive(:gitlab_shell).and_return(shell)
-      end
-
-      def expect_fork_repository
-        expect(shell).to receive(:fork_repository).with(project, forked_project)
+      def expect_fork_repository(success:)
+        allow(::Gitlab::GitalyClient::RepositoryService).to receive(:new).and_call_original
+        expect_next_instance_of(::Gitlab::GitalyClient::RepositoryService, forked_project.repository.raw) do |svc|
+          exp = expect(svc).to receive(:fork_repository).with(project.repository.raw)
+
+          if success
+            exp.and_return(true)
+          else
+            exp.and_raise(GRPC::BadStatus, 'Fork failed in tests')
+          end
+        end
       end
 
       describe 'when a worker was reset without cleanup' do
@@ -35,20 +39,20 @@ describe RepositoryForkWorker do
         it 'creates a new repository from a fork' do
           allow(subject).to receive(:jid).and_return(jid)
 
-          expect_fork_repository.and_return(true)
+          expect_fork_repository(success: true)
 
           perform!
         end
       end
 
       it "creates a new repository from a fork" do
-        expect_fork_repository.and_return(true)
+        expect_fork_repository(success: true)
 
         perform!
       end
 
       it 'protects the default branch' do
-        expect_fork_repository.and_return(true)
+        expect_fork_repository(success: true)
 
         perform!
 
@@ -56,7 +60,7 @@ describe RepositoryForkWorker do
       end
 
       it 'flushes various caches' do
-        expect_fork_repository.and_return(true)
+        expect_fork_repository(success: true)
 
         # Works around https://github.com/rspec/rspec-mocks/issues/910
         expect(Project).to receive(:find).with(forked_project.id).and_return(forked_project)
@@ -75,13 +79,13 @@ describe RepositoryForkWorker do
       it 'handles bad fork' do
         error_message = "Unable to fork project #{forked_project.id} for repository #{project.disk_path} -> #{forked_project.disk_path}: Failed to create fork repository"
 
-        expect_fork_repository.and_return(false)
+        expect_fork_repository(success: false)
 
         expect { perform! }.to raise_error(StandardError, error_message)
       end
 
       it 'calls Projects::LfsPointers::LfsLinkService#execute with OIDs of source project LFS objects' do
-        expect_fork_repository.and_return(true)
+        expect_fork_repository(success: true)
         expect_next_instance_of(Projects::LfsPointers::LfsLinkService) do |service|
           expect(service).to receive(:execute).with(project.all_lfs_objects_oids)
         end
@@ -92,7 +96,7 @@ describe RepositoryForkWorker do
       it "handles LFS objects link failure" do
         error_message = "Unable to fork project #{forked_project.id} for repository #{project.disk_path} -> #{forked_project.disk_path}: Source project has too many LFS objects"
 
-        expect_fork_repository.and_return(true)
+        expect_fork_repository(success: true)
         expect_next_instance_of(Projects::LfsPointers::LfsLinkService) do |service|
           expect(service).to receive(:execute).and_raise(Projects::LfsPointers::LfsLinkService::TooManyOidsError)
         end