Fix lib/gitlab/git

6 files changed, 181 insertions, 113 deletions

diff --git a/lib/gitlab/git/gitlab_projects.rb b/lib/gitlab/git/gitlab_projects.rb
index 00c943fdb25..8475645971e 100644
--- a/lib/gitlab/git/gitlab_projects.rb
+++ b/lib/gitlab/git/gitlab_projects.rb
@@ -53,24 +53,11 @@ module Gitlab
       # Import project via git clone --bare
       # URL must be publicly cloneable
       def import_project(source, timeout)
-        Gitlab::GitalyClient.migrate(:import_repository, status: Gitlab::GitalyClient::MigrationStatus::OPT_OUT) do |is_enabled|
-          if is_enabled
-            gitaly_import_repository(source)
-          else
-            git_import_repository(source, timeout)
-          end
-        end
+        git_import_repository(source, timeout)
       end
 
       def fork_repository(new_shard_name, new_repository_relative_path)
-        Gitlab::GitalyClient.migrate(:fork_repository,
-                                     status: Gitlab::GitalyClient::MigrationStatus::OPT_OUT) do |is_enabled|
-          if is_enabled
-            gitaly_fork_repository(new_shard_name, new_repository_relative_path)
-          else
-            git_fork_repository(new_shard_name, new_repository_relative_path)
-          end
-        end
+        git_fork_repository(new_shard_name, new_repository_relative_path)
       end
 
       def fetch_remote(name, timeout, force:, tags:, ssh_key: nil, known_hosts: nil, prune: true)
@@ -241,16 +228,6 @@ module Gitlab
         true
       end
 
-      def gitaly_import_repository(source)
-        raw_repository = Gitlab::Git::Repository.new(shard_name, repository_relative_path, nil)
-
-        Gitlab::GitalyClient::RepositoryService.new(raw_repository).import_repository(source)
-        true
-      rescue GRPC::BadStatus => e
-        @output << e.message
-        false
-      end
-
       def git_fork_repository(new_shard_name, new_repository_relative_path)
         from_path = repository_absolute_path
         new_shard_path = Gitlab.config.repositories.storages.fetch(new_shard_name).legacy_disk_path
@@ -270,16 +247,6 @@ module Gitlab
 
         run(cmd, nil) && Gitlab::Git::Repository.create_hooks(to_path, global_hooks_path)
       end
-
-      def gitaly_fork_repository(new_shard_name, new_repository_relative_path)
-        target_repository = Gitlab::Git::Repository.new(new_shard_name, new_repository_relative_path, nil)
-        raw_repository = Gitlab::Git::Repository.new(shard_name, repository_relative_path, nil)
-
-        Gitlab::GitalyClient::RepositoryService.new(target_repository).fork_repository(raw_repository)
-      rescue GRPC::BadStatus => e
-        logger.error "fork-repository failed: #{e.message}"
-        false
-      end
     end
   end
 end
diff --git a/lib/gitlab/shell.rb b/lib/gitlab/shell.rb
index 4a691d640b3..f6d82742e2b 100644
--- a/lib/gitlab/shell.rb
+++ b/lib/gitlab/shell.rb
@@ -106,8 +106,16 @@ module Gitlab
         raise Error.new("don't use disk paths with import_repository: #{url.inspect}")
       end
 
-      # The timeout ensures the subprocess won't hang forever
-      cmd = gitlab_projects(storage, "#{name}.git")
+      cmd = nil
+      relative_path = "#{name}.git"
+      Gitlab::GitalyClient.migrate(:import_repository, status: Gitlab::GitalyClient::MigrationStatus::OPT_OUT) do |is_enabled|
+        if is_enabled
+          cmd = GitalyGitlabProjects.new(storage, relative_path)
+        else
+          # The timeout ensures the subprocess won't hang forever
+          cmd = gitlab_projects(storage, relative_path)
+        end
+      end
       success = cmd.import_project(url, git_timeout)
 
       raise Error, cmd.output unless success
@@ -115,6 +123,7 @@ module Gitlab
       success
     end
 
+
     # Fetch remote for repository
     #
     # repository - an instance of Git::Repository
@@ -165,8 +174,19 @@ module Gitlab
     #
     # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/817
     def fork_repository(forked_from_storage, forked_from_disk_path, forked_to_storage, forked_to_disk_path)
-      gitlab_projects(forked_from_storage, "#{forked_from_disk_path}.git")
-        .fork_repository(forked_to_storage, "#{forked_to_disk_path}.git")
+      cmd = nil
+      forked_from_relative_path = "#{forked_from_disk_path}.git"
+
+      Gitlab::GitalyClient.migrate(:fork_repository,
+                                           status: Gitlab::GitalyClient::MigrationStatus::OPT_OUT) do |is_enabled|
+        if is_enabled
+          cmd = GitalyGitlabProjects.new(forked_from_storage, forked_from_relative_path)
+        else
+          cmd = gitlab_projects(forked_from_storage, forked_from_relative_path)
+        end
+      end
+
+      cmd.fork_repository(forked_to_storage, "#{forked_to_disk_path}.git")
     end
 
     # Removes a repository from file system, using rm_diretory which is an alias
@@ -452,5 +472,39 @@ module Gitlab
       # need to do the same here...
       raise Error, e
     end
+
+    class GitalyGitlabProjects
+      attr_reader :shard_name, :repository_relative_path, :output
+
+      def initialize(shard_name, repository_relative_path)
+        @shard_name = shard_name
+        @repository_relative_path = repository_relative_path
+        @output = ''
+      end
+
+      def import_project(source, _timeout)
+        raw_repository = Gitlab::Git::Repository.new(shard_name, repository_relative_path, nil)
+
+        Gitlab::GitalyClient::RepositoryService.new(raw_repository).import_repository(source)
+        true
+      rescue GRPC::BadStatus => e
+        @output = e.message
+        false
+      end
+
+      def fork_repository(new_shard_name, new_repository_relative_path)
+        target_repository = Gitlab::Git::Repository.new(new_shard_name, new_repository_relative_path, nil)
+        raw_repository = Gitlab::Git::Repository.new(shard_name, repository_relative_path, nil)
+
+        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
+      end
+    end
   end
 end
diff --git a/spec/lib/gitlab/git/branch_spec.rb b/spec/lib/gitlab/git/branch_spec.rb
index a19155ed5b0..ec1a684cfbc 100644
--- a/spec/lib/gitlab/git/branch_spec.rb
+++ b/spec/lib/gitlab/git/branch_spec.rb
@@ -69,7 +69,9 @@ describe Gitlab::Git::Branch, seed_helper: true do
       Gitlab::Git.committer_hash(email: user.email, name: user.name)
     end
     let(:params) do
-      parents = [repository.rugged.head.target]
+      parents = Gitlab::GitalyClient::StorageSettings.allow_disk_access do
+        [repository.rugged.head.target]
+      end
       tree = parents.first.tree
 
       {
diff --git a/spec/lib/gitlab/git/gitlab_projects_spec.rb b/spec/lib/gitlab/git/gitlab_projects_spec.rb
index 8b715d717c1..e5dd7244720 100644
--- a/spec/lib/gitlab/git/gitlab_projects_spec.rb
+++ b/spec/lib/gitlab/git/gitlab_projects_spec.rb
@@ -5,6 +5,13 @@ describe Gitlab::Git::GitlabProjects do
     TestEnv.clean_test_path
   end
 
+  around do |example|
+    # TODO move this spec to gitaly-ruby. GitlabProjects is not used in gitlab-ce
+    Gitlab::GitalyClient::StorageSettings.allow_disk_access do
+      example.run
+    end
+  end
+
   let(:project) { create(:project, :repository) }
 
   if $VERBOSE
@@ -190,36 +197,30 @@ describe Gitlab::Git::GitlabProjects do
       end
     end
 
-    context 'when Gitaly import_repository feature is enabled' do
-      it_behaves_like 'importing repository'
-    end
+    describe 'logging' do
+      it 'imports a repo' do
+        message = "Importing project from <#{import_url}> to <#{tmp_repo_path}>."
+        expect(logger).to receive(:info).with(message)
 
-    context 'when Gitaly import_repository feature is disabled', :disable_gitaly do
-      describe 'logging' do
-        it 'imports a repo' do
-          message = "Importing project from <#{import_url}> to <#{tmp_repo_path}>."
-          expect(logger).to receive(:info).with(message)
-
-          subject
-        end
+        subject
       end
+    end
 
-      context 'timeout' do
-        it 'does not import a repo' do
-          stub_spawn_timeout(cmd, timeout, nil)
+    context 'timeout' do
+      it 'does not import a repo' do
+        stub_spawn_timeout(cmd, timeout, nil)
 
-          message = "Importing project from <#{import_url}> to <#{tmp_repo_path}> failed."
-          expect(logger).to receive(:error).with(message)
+        message = "Importing project from <#{import_url}> to <#{tmp_repo_path}> failed."
+        expect(logger).to receive(:error).with(message)
 
-          is_expected.to be_falsy
+        is_expected.to be_falsy
 
-          expect(gl_projects.output).to eq("Timed out\n")
-          expect(File.exist?(File.join(tmp_repo_path, 'HEAD'))).to be_falsy
-        end
+        expect(gl_projects.output).to eq("Timed out\n")
+        expect(File.exist?(File.join(tmp_repo_path, 'HEAD'))).to be_falsy
       end
-
-      it_behaves_like 'importing repository'
     end
+
+    it_behaves_like 'importing repository'
   end
 
   describe '#fork_repository' do
@@ -258,51 +259,45 @@ describe Gitlab::Git::GitlabProjects do
       end
     end
 
-    context 'when Gitaly fork_repository feature is enabled' do
-      it_behaves_like 'forking a repository'
-    end
-
-    context 'when Gitaly fork_repository feature is disabled', :disable_gitaly do
-      it_behaves_like 'forking a repository'
+    it_behaves_like 'forking a repository'
 
-      # We seem to be stuck to having only one working Gitaly storage in tests, changing
-      # that is not very straight-forward so I'm leaving this test here for now till
-      # https://gitlab.com/gitlab-org/gitlab-ce/issues/41393 is fixed.
-      context 'different storages' do
-        let(:dest_repos) { 'alternative' }
-        let(:dest_repos_path) { File.join(File.dirname(tmp_repos_path), dest_repos) }
+    # We seem to be stuck to having only one working Gitaly storage in tests, changing
+    # that is not very straight-forward so I'm leaving this test here for now till
+    # https://gitlab.com/gitlab-org/gitlab-ce/issues/41393 is fixed.
+    context 'different storages' do
+      let(:dest_repos) { 'alternative' }
+      let(:dest_repos_path) { File.join(File.dirname(tmp_repos_path), dest_repos) }
 
-        before do
-          stub_storage_settings(dest_repos => { 'path' => dest_repos_path })
-        end
+      before do
+        stub_storage_settings(dest_repos => { 'path' => dest_repos_path })
+      end
 
-        it 'forks the repo' do
-          is_expected.to be_truthy
+      it 'forks the repo' do
+        is_expected.to be_truthy
 
-          expect(File.exist?(dest_repo)).to be_truthy
-          expect(File.exist?(File.join(dest_repo, 'hooks', 'pre-receive'))).to be_truthy
-          expect(File.exist?(File.join(dest_repo, 'hooks', 'post-receive'))).to be_truthy
-        end
+        expect(File.exist?(dest_repo)).to be_truthy
+        expect(File.exist?(File.join(dest_repo, 'hooks', 'pre-receive'))).to be_truthy
+        expect(File.exist?(File.join(dest_repo, 'hooks', 'post-receive'))).to be_truthy
       end
+    end
 
-      describe 'log messages' do
-        describe 'successful fork' do
-          it do
-            message = "Forking repository from <#{tmp_repo_path}> to <#{dest_repo}>."
-            expect(logger).to receive(:info).with(message)
+    describe 'log messages' do
+      describe 'successful fork' do
+        it do
+          message = "Forking repository from <#{tmp_repo_path}> to <#{dest_repo}>."
+          expect(logger).to receive(:info).with(message)
 
-            subject
-          end
+          subject
         end
+      end
 
-        describe 'failed fork due existing destination' do
-          it do
-            FileUtils.mkdir_p(dest_repo)
-            message = "fork-repository failed: destination repository <#{dest_repo}> already exists."
-            expect(logger).to receive(:error).with(message)
+      describe 'failed fork due existing destination' do
+        it do
+          FileUtils.mkdir_p(dest_repo)
+          message = "fork-repository failed: destination repository <#{dest_repo}> already exists."
+          expect(logger).to receive(:error).with(message)
 
-            subject
-          end
+          subject
         end
       end
     end
diff --git a/spec/lib/gitlab/git/hook_spec.rb b/spec/lib/gitlab/git/hook_spec.rb
index 2fe1f5603ce..c8a5052200c 100644
--- a/spec/lib/gitlab/git/hook_spec.rb
+++ b/spec/lib/gitlab/git/hook_spec.rb
@@ -8,6 +8,13 @@ describe Gitlab::Git::Hook do
     allow_any_instance_of(described_class).to receive(:trigger).and_call_original
   end
 
+  around do |example|
+    # TODO move hook tests to gitaly-ruby. Hook will disappear from gitlab-ce
+    Gitlab::GitalyClient::StorageSettings.allow_disk_access do
+      example.run
+    end
+  end
+
   describe "#trigger" do
     let(:project) { create(:project, :repository) }
     let(:repository) { project.repository.raw_repository }
diff --git a/spec/lib/gitlab/shell_spec.rb b/spec/lib/gitlab/shell_spec.rb
index 14eae22a2ec..a21790cc070 100644
--- a/spec/lib/gitlab/shell_spec.rb
+++ b/spec/lib/gitlab/shell_spec.rb
@@ -498,16 +498,34 @@ describe Gitlab::Shell do
         )
       end
 
-      it 'returns true when the command succeeds' do
-        expect(gitlab_projects).to receive(:fork_repository).with('nfs-file05', 'fork/path.git') { true }
+      context 'with gitaly' do
+        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
 
-        is_expected.to be_truthy
+        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
 
-      it 'return false when the command fails' do
-        expect(gitlab_projects).to receive(:fork_repository).with('nfs-file05', 'fork/path.git') { false }
+      context 'without gitaly', :disable_gitaly do
+        it 'returns true when the command succeeds' do
+          expect(gitlab_projects).to receive(:fork_repository).with('nfs-file05', 'fork/path.git') { true }
 
-        is_expected.to be_falsy
+          is_expected.to be_truthy
+        end
+
+        it 'return false when the command fails' do
+          expect(gitlab_projects).to receive(:fork_repository).with('nfs-file05', 'fork/path.git') { false }
+
+          is_expected.to be_falsy
+        end
       end
     end
 
@@ -662,21 +680,46 @@ describe Gitlab::Shell do
     describe '#import_repository' do
       let(:import_url) { 'https://gitlab.com/gitlab-org/gitlab-ce.git' }
 
-      it 'returns true when the command succeeds' do
-        expect(gitlab_projects).to receive(:import_project).with(import_url, timeout) { true }
 
-        result = gitlab_shell.import_repository(project.repository_storage, project.disk_path, import_url)
+      context 'with gitaly' do
+        it 'returns true when the command succeeds' do
+          expect_any_instance_of(Gitlab::GitalyClient::RepositoryService).to receive(:import_repository).with(import_url)
 
-        expect(result).to be_truthy
+          result = gitlab_shell.import_repository(project.repository_storage, project.disk_path, import_url)
+
+          expect(result).to be_truthy
+        end
+
+        it 'raises an exception when the command fails' do
+          expect_any_instance_of(Gitlab::GitalyClient::RepositoryService).to receive(:import_repository).with(import_url) { raise GRPC::BadStatus, 'bla' }
+          expect_any_instance_of(Gitlab::Shell::GitalyGitlabProjects).to receive(:output) { 'error'}
+
+          #allow(gitlab_projects).to receive(:output) { 'error' }
+          #expect(gitlab_projects).to receive(:import_project) { false }
+
+          expect do
+            gitlab_shell.import_repository(project.repository_storage, project.disk_path, import_url)
+          end.to raise_error(Gitlab::Shell::Error, "error")
+        end
       end
 
-      it 'raises an exception when the command fails' do
-        allow(gitlab_projects).to receive(:output) { 'error' }
-        expect(gitlab_projects).to receive(:import_project) { false }
+      context 'without gitaly', :disable_gitaly do
+        it 'returns true when the command succeeds' do
+          expect(gitlab_projects).to receive(:import_project).with(import_url, timeout) { true }
 
-        expect do
-          gitlab_shell.import_repository(project.repository_storage, project.disk_path, import_url)
-        end.to raise_error(Gitlab::Shell::Error, "error")
+          result = gitlab_shell.import_repository(project.repository_storage, project.disk_path, import_url)
+
+          expect(result).to be_truthy
+        end
+
+        it 'raises an exception when the command fails' do
+          allow(gitlab_projects).to receive(:output) { 'error' }
+          expect(gitlab_projects).to receive(:import_project) { false }
+
+          expect do
+            gitlab_shell.import_repository(project.repository_storage, project.disk_path, import_url)
+          end.to raise_error(Gitlab::Shell::Error, "error")
+        end
       end
     end
   end