Merge branch 'pks-checks-fix-overpushes-revalidating-commits' into 'master'

checks: Fix revalidation of preexisting commits

See merge request gitlab-org/gitlab!76106

lib/gitlab/checks/changes_access.rb

@@ -33,18 +33,33 @@ module Gitlab
       # changes. This set may also contain commits which are not referenced by
       # any of the new revisions.
       def commits
+        allow_quarantine = true
+
         newrevs = @changes.map do |change|
+          oldrev = change[:oldrev]
           newrev = change[:newrev]
-          newrev unless newrev.blank? || Gitlab::Git.blank_ref?(newrev)
+
+          next if blank_rev?(newrev)
+
+          # In case any of the old revisions is blank, then we cannot reliably
+          # detect which commits are new for a given change when enumerating
+          # objects via the object quarantine directory given that the client
+          # may have pushed too many commits, and we don't know when to
+          # terminate the walk. We thus fall back to using `git rev-list --not
+          # --all`, which is a lot less efficient but at least can only ever
+          # returns commits which really are new.
+          allow_quarantine = false if allow_quarantine && blank_rev?(oldrev)
+
+          newrev
         end.compact
 
         return [] if newrevs.empty?
 
-        @commits ||= project.repository.new_commits(newrevs, allow_quarantine: true)
+        @commits ||= project.repository.new_commits(newrevs, allow_quarantine: allow_quarantine)
       end
 
       # All commits which have been newly introduced via the given revision.
-      def commits_for(newrev)
+      def commits_for(oldrev, newrev)
         commits_by_id = commits.index_by(&:id)
 
         result = []
@@ -65,9 +80,11 @@ module Gitlab
 
           # Only add the parent ID to the pending set if we actually know its
           # commit to guards us against readding an ID which we have already
-          # queued up before.
+          # queued up before. Furthermore, we stop walking as soon as we hit
+          # `oldrev` such that we do not include any commits in our checks
+          # which have been "over-pushed" by the client.
           commit.parent_ids.each do |parent_id|
-            pending.add(parent_id) if commits_by_id.has_key?(parent_id)
+            pending.add(parent_id) if commits_by_id.has_key?(parent_id) && parent_id != oldrev
           end
 
           result << commit
@@ -80,10 +97,10 @@ module Gitlab
         @single_changes_accesses ||=
           changes.map do |change|
             commits =
-              if change[:newrev].blank? || Gitlab::Git.blank_ref?(change[:newrev])
+              if blank_rev?(change[:newrev])
                 []
               else
-                Gitlab::Lazy.new { commits_for(change[:newrev]) }
+                Gitlab::Lazy.new { commits_for(change[:oldrev], change[:newrev]) }
               end
 
             Checks::SingleChangeAccess.new(
@@ -109,6 +126,10 @@ module Gitlab
       def bulk_access_checks!
         Gitlab::Checks::LfsCheck.new(self).validate!
       end
+
+      def blank_rev?(rev)
+        rev.blank? || Gitlab::Git.blank_ref?(rev)
+      end
     end
   end
 end

spec/lib/gitlab/checks/changes_access_spec.rb

@@ -44,16 +44,30 @@ RSpec.describe Gitlab::Checks::ChangesAccess do
     it 'calls #new_commits' do
       expect(project.repository).to receive(:new_commits).and_call_original
 
-      expect(subject.commits).to eq([])
+      expect(subject.commits).to match_array([])
     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], { allow_quarantine: true }) { [expected_commit] }
-        expect(subject.commits).to eq([expected_commit])
+      shared_examples 'returns only commits with non empty revisions' do
+        specify do
+          expect(project.repository).to receive(:new_commits).with([newrev], { allow_quarantine: allow_quarantine }) { [expected_commit] }
+          expect(subject.commits).to match_array([expected_commit])
+        end
+      end
+
+      it_behaves_like 'returns only commits with non empty revisions' do
+        let(:changes) { [{ oldrev: oldrev, newrev: newrev }, { newrev: '' }, { newrev: Gitlab::Git::BLANK_SHA }] }
+        let(:allow_quarantine) { true }
+      end
+
+      context 'without oldrev' do
+        it_behaves_like 'returns only commits with non empty revisions' do
+          let(:changes) { [{ newrev: newrev }, { newrev: '' }, { newrev: Gitlab::Git::BLANK_SHA }] }
+          # The quarantine directory should not be used because we're lacking oldrev.
+          let(:allow_quarantine) { false }
+        end
       end
     end
   end
@@ -61,12 +75,13 @@ RSpec.describe Gitlab::Checks::ChangesAccess do
   describe '#commits_for' do
     let(:new_commits) { [] }
     let(:expected_commits) { [] }
+    let(:oldrev) { Gitlab::Git::BLANK_SHA }
 
     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)
+        expect(subject.commits_for(oldrev, newrev)).to eq(expected_commits)
       end
     end
 
@@ -172,6 +187,31 @@ RSpec.describe Gitlab::Checks::ChangesAccess do
 
       it_behaves_like 'a listing of new commits'
     end
+
+    context 'with over-push' do
+      let(:newrev) { '1' }
+      let(:oldrev) { '3' }
+
+      # `#new_commits` returns too many commits, where some commits are not
+      # part of the current change.
+      let(:new_commits) do
+        [
+          create_commit('1', %w[2]),
+          create_commit('2', %w[3]),
+          create_commit('3', %w[4]),
+          create_commit('4', %w[])
+        ]
+      end
+
+      let(:expected_commits) do
+        [
+          create_commit('1', %w[2]),
+          create_commit('2', %w[3])
+        ]
+      end
+
+      it_behaves_like 'a listing of new commits'
+    end
   end
 
   describe '#single_change_accesses' do
@@ -180,10 +220,10 @@ RSpec.describe Gitlab::Checks::ChangesAccess do
 
     shared_examples '#single_change_access' do
       before do
-        commits_for.each do |id, commits|
+        commits_for.each do |oldrev, newrev, commits|
           expect(subject)
             .to receive(:commits_for)
-            .with(id)
+            .with(oldrev, newrev)
             .and_return(commits)
         end
       end
@@ -205,7 +245,12 @@ RSpec.describe Gitlab::Checks::ChangesAccess do
     end
 
     context 'with a single change and no new commits' do
-      let(:commits_for) { { 'new' => [] } }
+      let(:commits_for) do
+        [
+          ['old', 'new', []]
+        ]
+      end
+
       let(:changes) do
         [
           { oldrev: 'old', newrev: 'new', ref: 'refs/heads/branch' }
@@ -222,7 +267,12 @@ RSpec.describe Gitlab::Checks::ChangesAccess do
     end
 
     context 'with a single change and new commits' do
-      let(:commits_for) { { 'new' => [create_commit('new', [])] } }
+      let(:commits_for) do
+        [
+          ['old', 'new', [create_commit('new', [])]]
+        ]
+      end
+
       let(:changes) do
         [
           { oldrev: 'old', newrev: 'new', ref: 'refs/heads/branch' }
@@ -240,11 +290,11 @@ RSpec.describe Gitlab::Checks::ChangesAccess do
 
     context 'with multiple changes' do
       let(:commits_for) do
-        {
-          'a' => [create_commit('a', [])],
-          'c' => [create_commit('c', [])],
-          'd' => []
-        }
+        [
+          [nil, 'a', [create_commit('a', [])]],
+          ['a', 'c', [create_commit('c', [])]],
+          [nil, 'd', []]
+        ]
       end
 
       let(:changes) do