Merge branch 'pks-checks-quarantined-commits' into 'master'

checks: Speed up retrieving commits via quarantine directory

See merge request gitlab-org/gitlab!67210

lib/gitlab/checks/changes_access.rb

@@ -40,7 +40,7 @@ module Gitlab
 
         return [] if newrevs.empty?
 
-        @commits ||= project.repository.new_commits(newrevs)
+        @commits ||= project.repository.new_commits(newrevs, allow_quarantine: true)
       end
 
       # All commits which have been newly introduced via the given revision.

lib/gitlab/git/repository.rb

@@ -354,9 +354,9 @@ module Gitlab
         end
       end
 
-      def new_commits(newrevs)
+      def new_commits(newrevs, allow_quarantine: false)
         wrapped_gitaly_errors do
-          gitaly_commit_client.list_commits(Array.wrap(newrevs) + %w[--not --all])
+          gitaly_commit_client.list_new_commits(Array.wrap(newrevs), allow_quarantine: allow_quarantine)
         end
       end
 

lib/gitlab/gitaly_client/commit_service.rb

@@ -265,6 +265,31 @@ module Gitlab
         consume_commits_response(response)
       end
 
+      # List all commits which are new in the repository. If commits have been pushed into the repo
+      def list_new_commits(revisions, allow_quarantine: false)
+        git_env = Gitlab::Git::HookEnv.all(@gitaly_repo.gl_repository)
+        if allow_quarantine && git_env['GIT_OBJECT_DIRECTORY_RELATIVE'].present?
+          # If we have a quarantine environment, then we can optimize the check
+          # by doing a ListAllCommitsRequest. Instead of walking through
+          # references, we just walk through all quarantined objects, which is
+          # a lot more efficient. To do so, we throw away any alternate object
+          # directories, which point to the main object directory of the
+          # repository, and only keep the object directory which points into
+          # the quarantine object directory.
+          quarantined_repo = @gitaly_repo.dup
+          quarantined_repo.git_alternate_object_directories = Google::Protobuf::RepeatedField.new(:string)
+
+          request = Gitaly::ListAllCommitsRequest.new(
+            repository: quarantined_repo
+          )
+
+          response = GitalyClient.call(@repository.storage, :commit_service, :list_all_commits, request, timeout: GitalyClient.medium_timeout)
+          consume_commits_response(response)
+        else
+          list_commits(Array.wrap(revisions) + %w[--not --all])
+        end
+      end
+
       def list_commits_by_oid(oids)
         return [] if oids.empty?
 

spec/lib/gitlab/checks/changes_access_spec.rb

@@ -70,7 +70,7 @@ RSpec.describe Gitlab::Checks::ChangesAccess do
       let(:expected_commit) { instance_double(Commit) }
 
       it 'returns only commits with non empty revisions' do
-        expect(project.repository).to receive(:new_commits).with([newrev]) { [expected_commit] }
+        expect(project.repository).to receive(:new_commits).with([newrev], { allow_quarantine: true }) { [expected_commit] }
         expect(subject.commits).to eq([expected_commit])
       end
     end

spec/lib/gitlab/gitaly_client/commit_service_spec.rb

@@ -343,6 +343,92 @@ RSpec.describe Gitlab::GitalyClient::CommitService do
     end
   end
 
+  describe '#list_new_commits' do
+    let(:revisions) { [revision] }
+    let(:gitaly_commits) { create_list(:gitaly_commit, 3) }
+    let(:commits) { gitaly_commits.map { |c| Gitlab::Git::Commit.new(repository, c) }}
+
+    subject { client.list_new_commits(revisions, allow_quarantine: allow_quarantine) }
+
+    shared_examples 'a #list_all_commits message' do
+      it 'sends a list_all_commits message' do
+        expected_repository = repository.gitaly_repository.dup
+        expected_repository.git_alternate_object_directories = Google::Protobuf::RepeatedField.new(:string)
+
+        expect_next_instance_of(Gitaly::CommitService::Stub) do |service|
+          expect(service).to receive(:list_all_commits)
+            .with(gitaly_request_with_params(repository: expected_repository), kind_of(Hash))
+            .and_return([Gitaly::ListAllCommitsResponse.new(commits: gitaly_commits)])
+        end
+
+        expect(subject).to eq(commits)
+      end
+    end
+
+    shared_examples 'a #list_commits message' do
+      it 'sends a list_commits message' do
+        expect_next_instance_of(Gitaly::CommitService::Stub) do |service|
+          expect(service).to receive(:list_commits)
+            .with(gitaly_request_with_params(revisions: revisions + %w[--not --all]), kind_of(Hash))
+            .and_return([Gitaly::ListCommitsResponse.new(commits: gitaly_commits)])
+        end
+
+        expect(subject).to eq(commits)
+      end
+    end
+
+    before do
+      ::Gitlab::GitalyClient.clear_stubs!
+
+      allow(Gitlab::Git::HookEnv)
+        .to receive(:all)
+        .with(repository.gl_repository)
+        .and_return(git_env)
+    end
+
+    context 'with hook environment' do
+      let(:git_env) do
+        {
+          'GIT_OBJECT_DIRECTORY_RELATIVE' => '.git/objects',
+          'GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE' => ['/dir/one', '/dir/two']
+        }
+      end
+
+      context 'with allowed quarantine' do
+        let(:allow_quarantine) { true }
+
+        it_behaves_like 'a #list_all_commits message'
+      end
+
+      context 'with disallowed quarantine' do
+        let(:allow_quarantine) { false }
+
+        it_behaves_like 'a #list_commits message'
+      end
+    end
+
+    context 'without hook environment' do
+      let(:git_env) do
+        {
+          'GIT_OBJECT_DIRECTORY_RELATIVE' => '',
+          'GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE' => []
+        }
+      end
+
+      context 'with allowed quarantine' do
+        let(:allow_quarantine) { true }
+
+        it_behaves_like 'a #list_commits message'
+      end
+
+      context 'with disallowed quarantine' do
+        let(:allow_quarantine) { false }
+
+        it_behaves_like 'a #list_commits message'
+      end
+    end
+  end
+
   describe '#commit_stats' do
     let(:request) do
       Gitaly::CommitStatsRequest.new(