Merge branch 'pks-bulk-diff-checks' into 'master'

checks: Implement infrastructure for bulk diff checks

See merge request gitlab-org/gitlab!66731

config/feature_flags/development/changes_batch_commits.yml

+---
+name: changes_batch_commits
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/66731
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/336992
+milestone: '14.2'
+type: development
+group: group::gitaly
+default_enabled: false

ee/spec/lib/gitlab/git_access_spec.rb

@@ -77,7 +77,7 @@ RSpec.describe Gitlab::GitAccess do
       end
 
       it 'allows githook for new branch with an old bad commit' do
-        bad_commit = double("Commit", safe_message: 'Some change').as_null_object
+        bad_commit = double("Commit", safe_message: 'Some change', id: end_sha).as_null_object
         ref_object = double(name: 'heads/master')
         allow(bad_commit).to receive(:refs).and_return([ref_object])
         allow_next_instance_of(Repository) do |instance|
@@ -91,7 +91,7 @@ RSpec.describe Gitlab::GitAccess do
       end
 
       it 'allows githook for any change with an old bad commit' do
-        bad_commit = double("Commit", safe_message: 'Some change').as_null_object
+        bad_commit = double("Commit", safe_message: 'Some change', id: end_sha).as_null_object
         ref_object = double(name: 'heads/master')
         allow(bad_commit).to receive(:refs).and_return([ref_object])
         allow(project.repository).to receive(:commits_between).and_return([bad_commit])
@@ -103,7 +103,7 @@ RSpec.describe Gitlab::GitAccess do
       end
 
       it 'does not allow any change from Web UI with bad commit' do
-        bad_commit = double("Commit", safe_message: 'Some change').as_null_object
+        bad_commit = double("Commit", safe_message: 'Some change', id: end_sha).as_null_object
         # We use tmp ref a a temporary for Web UI commiting
         ref_object = double(name: 'refs/tmp')
         allow(bad_commit).to receive(:refs).and_return([ref_object])

lib/gitlab/checks/changes_access.rb

@@ -29,11 +29,48 @@ module Gitlab
         true
       end
 
+      # All commits which have been newly introduced via any of the given
+      # changes. This set may also contain commits which are not referenced by
+      # any of the new revisions.
+      def commits
+        newrevs = @changes.map do |change|
+          newrev = change[:newrev]
+          newrev unless newrev.blank? || Gitlab::Git.blank_ref?(newrev)
+        end.compact
+
+        return [] if newrevs.empty?
+
+        @commits ||= project.repository.new_commits(newrevs)
+      end
+
+      # All commits which have been newly introduced via the given revision.
+      def commits_for(newrev)
+        commits_by_id = commits.index_by(&:id)
+
+        result = []
+        pending = [newrev]
+
+        # We go up the parent chain of our newrev and collect all commits which
+        # are new. In case a commit's ID cannot be found in the set of new
+        # commits, then it must already be a preexisting commit.
+        pending.each do |rev|
+          commit = commits_by_id[rev]
+          next if commit.nil?
+
+          pending.push(*commit.parent_ids)
+          result << commit
+        end
+
+        result
+      end
+
       protected
 
       def single_access_checks!
         # Iterate over all changes to find if user allowed all of them to be applied
         changes.each do |change|
+          commits = Gitlab::Lazy.new { commits_for(change[:newrev]) } if Feature.enabled?(:changes_batch_commits)
+
           # If user does not have access to make at least one change, cancel all
           # push by allowing the exception to bubble up
           Checks::SingleChangeAccess.new(
@@ -41,7 +78,8 @@ module Gitlab
             user_access: user_access,
             project: project,
             protocol: protocol,
-            logger: logger
+            logger: logger,
+            commits: commits
           ).validate!
         end
       end

lib/gitlab/checks/single_change_access.rb

@@ -11,7 +11,7 @@ module Gitlab
 
       def initialize(
         change, user_access:, project:,
-        protocol:, logger:
+        protocol:, logger:, commits: nil
       )
         @oldrev, @newrev, @ref = change.values_at(:oldrev, :newrev, :ref)
         @branch_name = Gitlab::Git.branch_name(@ref)
@@ -19,6 +19,7 @@ module Gitlab
         @user_access = user_access
         @project = project
         @protocol = protocol
+        @commits = commits
 
         @logger = logger
         @logger.append_message("Running checks for ref: #{@branch_name || @tag_name}")

spec/lib/gitlab/checks/changes_access_spec.rb

@@ -3,40 +3,169 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Checks::ChangesAccess do
+  include_context 'changes access checks context'
+
+  subject { changes_access }
+
   describe '#validate!' do
-    include_context 'changes access checks context'
+    shared_examples '#validate!' do
+      before do
+        allow(project).to receive(:lfs_enabled?).and_return(true)
+      end
 
-    before do
-      allow(project).to receive(:lfs_enabled?).and_return(true)
-    end
+      context 'without failed checks' do
+        it "doesn't raise an error" do
+          expect { subject.validate! }.not_to raise_error
+        end
 
-    subject { changes_access }
+        it 'calls lfs checks' do
+          expect_next_instance_of(Gitlab::Checks::LfsCheck) do |instance|
+            expect(instance).to receive(:validate!)
+          end
 
-    context 'without failed checks' do
-      it "doesn't raise an error" do
-        expect { subject.validate! }.not_to raise_error
+          subject.validate!
+        end
       end
 
-      it 'calls lfs checks' do
-        expect_next_instance_of(Gitlab::Checks::LfsCheck) do |instance|
-          expect(instance).to receive(:validate!)
+      context 'when time limit was reached' do
+        it 'raises a TimeoutError' do
+          logger = Gitlab::Checks::TimedLogger.new(start_time: timeout.ago, timeout: timeout)
+          access = described_class.new(changes,
+                                       project: project,
+                                       user_access: user_access,
+                                       protocol: protocol,
+                                       logger: logger)
+
+          expect { access.validate! }.to raise_error(Gitlab::Checks::TimedLogger::TimeoutError)
         end
+      end
+    end
+
+    context 'with batched commits enabled' do
+      before do
+        stub_feature_flags(changes_batch_commits: true)
+      end
+
+      it_behaves_like '#validate!'
+    end
+
+    context 'with batched commits disabled' do
+      before do
+        stub_feature_flags(changes_batch_commits: false)
+      end
+
+      it_behaves_like '#validate!'
+    end
+  end
+
+  describe '#commits' do
+    it 'calls #new_commits' do
+      expect(project.repository).to receive(:new_commits).and_return([])
+
+      expect(subject.commits).to eq([])
+    end
+
+    context 'when changes contain empty revisions' do
+      let(:changes) { [{ newrev: newrev }, { newrev: '' }, { newrev: Gitlab::Git::BLANK_SHA }] }
+      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(subject.commits).to eq([expected_commit])
+      end
+    end
+  end
 
-        subject.validate!
+  describe '#commits_for' do
+    let(:new_commits) { [] }
+    let(:expected_commits) { [] }
+
+    shared_examples 'a listing of new commits' do
+      it 'returns expected commits' do
+        expect(subject).to receive(:commits).and_return(new_commits)
+
+        expect(subject.commits_for(newrev)).to eq(expected_commits)
+      end
+    end
+
+    context 'with no commits' do
+      it_behaves_like 'a listing of new commits'
+    end
+
+    context 'with unrelated commits' do
+      let(:new_commits) { [create_commit('1234', %w[1111 2222])] }
+
+      it_behaves_like 'a listing of new commits'
+    end
+
+    context 'with single related commit' do
+      let(:new_commits) { [create_commit(newrev, %w[1111 2222])] }
+      let(:expected_commits) { new_commits }
+
+      it_behaves_like 'a listing of new commits'
+    end
+
+    context 'with single related and unrelated commit' do
+      let(:new_commits) do
+        [
+          create_commit(newrev, %w[1111 2222]),
+          create_commit('abcd', %w[1111 2222])
+        ]
       end
+
+      let(:expected_commits) do
+        [create_commit(newrev, %w[1111 2222])]
+      end
+
+      it_behaves_like 'a listing of new commits'
     end
 
-    context 'when time limit was reached' do
-      it 'raises a TimeoutError' do
-        logger = Gitlab::Checks::TimedLogger.new(start_time: timeout.ago, timeout: timeout)
-        access = described_class.new(changes,
-                                     project: project,
-                                     user_access: user_access,
-                                     protocol: protocol,
-                                     logger: logger)
+    context 'with multiple related commits' do
+      let(:new_commits) do
+        [
+          create_commit(newrev, %w[1111]),
+          create_commit('1111', %w[2222]),
+          create_commit('abcd', [])
+        ]
+      end
 
-        expect { access.validate! }.to raise_error(Gitlab::Checks::TimedLogger::TimeoutError)
+      let(:expected_commits) do
+        [
+          create_commit(newrev, %w[1111]),
+          create_commit('1111', %w[2222])
+        ]
       end
+
+      it_behaves_like 'a listing of new commits'
     end
+
+    context 'with merge commits' do
+      let(:new_commits) do
+        [
+          create_commit(newrev, %w[1111 2222 3333]),
+          create_commit('1111', []),
+          create_commit('3333', %w[4444]),
+          create_commit('4444', [])
+        ]
+      end
+
+      let(:expected_commits) do
+        [
+          create_commit(newrev, %w[1111 2222 3333]),
+          create_commit('1111', []),
+          create_commit('3333', %w[4444]),
+          create_commit('4444', [])
+        ]
+      end
+
+      it_behaves_like 'a listing of new commits'
+    end
+  end
+
+  def create_commit(id, parent_ids)
+    Gitlab::Git::Commit.new(project.repository, {
+      id: id,
+      parent_ids: parent_ids
+    })
   end
 end

spec/lib/gitlab/checks/single_change_access_spec.rb

@@ -58,5 +58,52 @@ RSpec.describe Gitlab::Checks::SingleChangeAccess do
         expect { access.validate! }.to raise_error(Gitlab::Checks::TimedLogger::TimeoutError)
       end
     end
+
+    describe '#commits' do
+      let(:expected_commits) { [Gitlab::Git::Commit.new(project.repository, { id: "1234" })] }
+
+      let(:access) do
+        described_class.new(changes,
+                            project: project,
+                            user_access: user_access,
+                            protocol: protocol,
+                            logger: logger,
+                            commits: provided_commits)
+      end
+
+      shared_examples '#commits' do
+        it 'returns expected commits' do
+          expect(access.commits).to eq(expected_commits)
+        end
+
+        it 'returns expected commits on repeated calls' do
+          expect(access.commits).to eq(expected_commits)
+          expect(access.commits).to eq(expected_commits)
+        end
+      end
+
+      context 'with provided commits' do
+        let(:provided_commits) { expected_commits }
+
+        before do
+          expect(project.repository).not_to receive(:new_commits)
+        end
+
+        it_behaves_like '#commits'
+      end
+
+      context 'without provided commits' do
+        let(:provided_commits) { nil }
+
+        before do
+          expect(project.repository)
+            .to receive(:new_commits)
+            .once
+            .and_return(expected_commits)
+        end
+
+        it_behaves_like '#commits'
+      end
+    end
   end
 end