Merge branch 'zj-calculate-checksum-mandator' into 'master'

Calculating repository checksums executed by Gitaly

Closes gitaly#1105 and #46293

See merge request gitlab-org/gitlab-ce!18907

3 files changed, 35 insertions, 86 deletions

diff --git a/changelogs/unreleased/zj-calculate-checksum-mandator.yml b/changelogs/unreleased/zj-calculate-checksum-mandator.yml
new file mode 100644
index 00000000000..83315a3c5dd
--- /dev/null
+++ b/changelogs/unreleased/zj-calculate-checksum-mandator.yml
@@ -0,0 +1,5 @@
+---
+title: Remove shellout implementation for Repository checksums
+merge_request:
+author:
+type: other
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index 061865a7acf..b521d69930a 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -1572,14 +1572,12 @@ module Gitlab
       end
 
       def checksum
-        gitaly_migrate(:calculate_checksum,
-                       status: Gitlab::GitalyClient::MigrationStatus::OPT_OUT) do |is_enabled|
-          if is_enabled
-            gitaly_repository_client.calculate_checksum
-          else
-            calculate_checksum_by_shelling_out
-          end
-        end
+        # The exists? RPC is much cheaper, so we perform this request first
+        raise NoRepository, "Repository does not exists" unless exists?
+
+        gitaly_repository_client.calculate_checksum
+      rescue GRPC::NotFound
+        raise NoRepository # Guard against data races.
       end
 
       private
@@ -2478,36 +2476,6 @@ module Gitlab
         rev_parse_target(ref).oid
       end
 
-      def calculate_checksum_by_shelling_out
-        raise NoRepository unless exists?
-
-        args = %W(--git-dir=#{path} show-ref --heads --tags)
-        output, status = run_git(args)
-
-        if status.nil? || !status.zero?
-          # Non-valid git repositories return 128 as the status code and an error output
-          raise InvalidRepository if status == 128 && output.to_s.downcase =~ /not a git repository/
-          # Empty repositories returns with a non-zero status and an empty output.
-          raise ChecksumError, output unless output.blank?
-
-          return EMPTY_REPOSITORY_CHECKSUM
-        end
-
-        refs = output.split("\n")
-
-        result = refs.inject(nil) do |checksum, ref|
-          value = Digest::SHA1.hexdigest(ref).hex
-
-          if checksum.nil?
-            value
-          else
-            checksum ^ value
-          end
-        end
-
-        result.to_s(16)
-      end
-
       def build_git_cmd(*args)
         object_directories = alternate_object_directories.join(File::PATH_SEPARATOR)
 
diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb
index fcb690d8aa3..2b5710ac401 100644
--- a/spec/lib/gitlab/git/repository_spec.rb
+++ b/spec/lib/gitlab/git/repository_spec.rb
@@ -2247,66 +2247,42 @@ describe Gitlab::Git::Repository, seed_helper: true do
   end
 
   describe '#checksum' do
-    shared_examples 'calculating checksum' do
-      it 'calculates the checksum for non-empty repo' do
-        expect(repository.checksum).to eq '54f21be4c32c02f6788d72207fa03ad3bce725e4'
-      end
-
-      it 'returns 0000000000000000000000000000000000000000 for an empty repo' do
-        FileUtils.rm_rf(File.join(storage_path, 'empty-repo.git'))
-
-        system(git_env, *%W(#{Gitlab.config.git.bin_path} init --bare empty-repo.git),
-               chdir: storage_path,
-               out:   '/dev/null',
-               err:   '/dev/null')
+    it 'calculates the checksum for non-empty repo' do
+      expect(repository.checksum).to eq '54f21be4c32c02f6788d72207fa03ad3bce725e4'
+    end
 
-        empty_repo = described_class.new('default', 'empty-repo.git', '')
+    it 'returns 0000000000000000000000000000000000000000 for an empty repo' do
+      FileUtils.rm_rf(File.join(storage_path, 'empty-repo.git'))
 
-        expect(empty_repo.checksum).to eq '0000000000000000000000000000000000000000'
-      end
+      system(git_env, *%W(#{Gitlab.config.git.bin_path} init --bare empty-repo.git),
+             chdir: storage_path,
+             out:   '/dev/null',
+             err:   '/dev/null')
 
-      it 'raises Gitlab::Git::Repository::InvalidRepository error for non-valid git repo' do
-        FileUtils.rm_rf(File.join(storage_path, 'non-valid.git'))
+      empty_repo = described_class.new('default', 'empty-repo.git', '')
 
-        system(git_env, *%W(#{Gitlab.config.git.bin_path} clone --bare #{TEST_REPO_PATH} non-valid.git),
-               chdir: SEED_STORAGE_PATH,
-               out: '/dev/null',
-               err: '/dev/null')
+      expect(empty_repo.checksum).to eq '0000000000000000000000000000000000000000'
+    end
 
-        File.truncate(File.join(storage_path, 'non-valid.git/HEAD'), 0)
+    it 'raises Gitlab::Git::Repository::InvalidRepository error for non-valid git repo' do
+      FileUtils.rm_rf(File.join(storage_path, 'non-valid.git'))
 
-        non_valid = described_class.new('default', 'non-valid.git', '')
+      system(git_env, *%W(#{Gitlab.config.git.bin_path} clone --bare #{TEST_REPO_PATH} non-valid.git),
+             chdir: SEED_STORAGE_PATH,
+             out: '/dev/null',
+             err: '/dev/null')
 
-        expect { non_valid.checksum }.to raise_error(Gitlab::Git::Repository::InvalidRepository)
-      end
+      File.truncate(File.join(storage_path, 'non-valid.git/HEAD'), 0)
 
-      it 'raises Gitlab::Git::Repository::NoRepository error when there is no repo' do
-        broken_repo = described_class.new('default', 'a/path.git', '')
+      non_valid = described_class.new('default', 'non-valid.git', '')
 
-        expect { broken_repo.checksum }.to raise_error(Gitlab::Git::Repository::NoRepository)
-      end
+      expect { non_valid.checksum }.to raise_error(Gitlab::Git::Repository::InvalidRepository)
     end
 
-    context 'when calculate_checksum Gitaly feature is enabled' do
-      it_behaves_like 'calculating checksum'
-    end
-
-    context 'when calculate_checksum Gitaly feature is disabled', :disable_gitaly do
-      it_behaves_like 'calculating checksum'
-
-      describe 'when storage is broken', :broken_storage  do
-        it 'raises a storage exception when storage is not available' do
-          broken_repo = described_class.new('broken', 'a/path.git', '')
-
-          expect { broken_repo.rugged }.to raise_error(Gitlab::Git::Storage::Inaccessible)
-        end
-      end
-
-      it "raises a Gitlab::Git::Repository::Failure error if the `popen` call to git returns a non-zero exit code" do
-        allow(repository).to receive(:popen).and_return(['output', nil])
+    it 'raises Gitlab::Git::Repository::NoRepository error when there is no repo' do
+      broken_repo = described_class.new('default', 'a/path.git', '')
 
-        expect { repository.checksum }.to raise_error Gitlab::Git::Repository::ChecksumError
-      end
+      expect { broken_repo.checksum }.to raise_error(Gitlab::Git::Repository::NoRepository)
     end
   end