Skip per-commit validations which have already passed on another change/branch

7 files changed, 90 insertions, 16 deletions

diff --git a/app/models/commit.rb b/app/models/commit.rb
index a422a0995ff..01f4c58daa1 100644
--- a/app/models/commit.rb
+++ b/app/models/commit.rb
@@ -469,6 +469,10 @@ class Commit
     !!merged_merge_request(user)
   end
 
+  def cache_key
+    "commit:#{sha}"
+  end
+
   private
 
   def commit_reference(from, referable_commit_id, full: false)
diff --git a/changelogs/unreleased/fj-44679-skip-per-commit-validations.yml b/changelogs/unreleased/fj-44679-skip-per-commit-validations.yml
new file mode 100644
index 00000000000..3f9754409df
--- /dev/null
+++ b/changelogs/unreleased/fj-44679-skip-per-commit-validations.yml
@@ -0,0 +1,5 @@
+---
+title: Skip per-commit validations already evaluated
+merge_request: 23984
+author:
+type: performance
diff --git a/lib/gitlab/checks/base_checker.rb b/lib/gitlab/checks/base_checker.rb
index f8cda0382fe..7fbcf6a4ff4 100644
--- a/lib/gitlab/checks/base_checker.rb
+++ b/lib/gitlab/checks/base_checker.rb
@@ -33,6 +33,22 @@ module Gitlab
       def tag_exists?
         project.repository.tag_exists?(tag_name)
       end
+
+      def validate_once(resource)
+        Gitlab::SafeRequestStore.fetch(cache_key_for_resource(resource)) do
+          yield(resource)
+
+          true
+        end
+      end
+
+      def cache_key_for_resource(resource)
+        "git_access:#{checker_cache_key}:#{resource.cache_key}"
+      end
+
+      def checker_cache_key
+        self.class.name.demodulize.underscore
+      end
     end
   end
 end
diff --git a/lib/gitlab/checks/diff_check.rb b/lib/gitlab/checks/diff_check.rb
index 8ee345ab45a..63da9a3d6b5 100644
--- a/lib/gitlab/checks/diff_check.rb
+++ b/lib/gitlab/checks/diff_check.rb
@@ -14,13 +14,17 @@ module Gitlab
         return if deletion? || newrev.nil?
         return unless should_run_diff_validations?
         return if commits.empty?
-        return unless uses_raw_delta_validations?
 
         file_paths = []
-        process_raw_deltas do |diff|
-          file_paths << (diff.new_path || diff.old_path)
 
-          validate_diff(diff)
+        process_commits do |commit|
+          validate_once(commit) do
+            commit.raw_deltas.each do |diff|
+              file_paths << (diff.new_path || diff.old_path)
+
+              validate_diff(diff)
+            end
+          end
         end
 
         validate_file_paths(file_paths)
@@ -28,17 +32,13 @@ module Gitlab
 
       private
 
-      def should_run_diff_validations?
-        validate_lfs_file_locks?
-      end
-
       def validate_lfs_file_locks?
         strong_memoize(:validate_lfs_file_locks) do
           project.lfs_enabled? && project.any_lfs_file_locks?
         end
       end
 
-      def uses_raw_delta_validations?
+      def should_run_diff_validations?
         validations_for_diff.present? || path_validations.present?
       end
 
@@ -59,16 +59,14 @@ module Gitlab
         validate_lfs_file_locks? ? [lfs_file_locks_validation] : []
       end
 
-      def process_raw_deltas
+      def process_commits
         logger.log_timed(LOG_MESSAGES[:diff_content_check]) do
           # n+1: https://gitlab.com/gitlab-org/gitlab-ee/issues/3593
           ::Gitlab::GitalyClient.allow_n_plus_1_calls do
             commits.each do |commit|
               logger.check_timeout_reached
 
-              commit.raw_deltas.each do |diff|
-                yield(diff)
-              end
+              yield(commit)
             end
           end
         end
diff --git a/lib/gitlab/checks/lfs_check.rb b/lib/gitlab/checks/lfs_check.rb
index e42684e679a..cc6a14d2d9a 100644
--- a/lib/gitlab/checks/lfs_check.rb
+++ b/lib/gitlab/checks/lfs_check.rb
@@ -7,6 +7,7 @@ module Gitlab
       ERROR_MESSAGE = 'LFS objects are missing. Ensure LFS is properly set up or try a manual "git lfs push --all".'.freeze
 
       def validate!
+        return unless project.lfs_enabled?
         return if skip_lfs_integrity_check
 
         logger.log_timed(LOG_MESSAGE) do
diff --git a/spec/lib/gitlab/checks/diff_check_spec.rb b/spec/lib/gitlab/checks/diff_check_spec.rb
index eeec1e83179..a341dfa5636 100644
--- a/spec/lib/gitlab/checks/diff_check_spec.rb
+++ b/spec/lib/gitlab/checks/diff_check_spec.rb
@@ -47,5 +47,43 @@ describe Gitlab::Checks::DiffCheck do
         end
       end
     end
+
+    context 'commit diff validations' do
+      before do
+        allow(subject).to receive(:validations_for_diff).and_return([lambda { |diff| return }])
+
+        expect_any_instance_of(Commit).to receive(:raw_deltas).and_call_original
+
+        subject.validate!
+      end
+
+      context 'when request store is inactive' do
+        it 'are run for every commit' do
+          expect_any_instance_of(Commit).to receive(:raw_deltas).and_call_original
+
+          subject.validate!
+        end
+      end
+
+      context 'when request store is active', :request_store do
+        it 'are cached for every commit' do
+          expect_any_instance_of(Commit).not_to receive(:raw_deltas)
+
+          subject.validate!
+        end
+
+        it 'are run for not cached commits' do
+          allow(project.repository).to receive(:new_commits).and_return(
+            project.repository.commits_between('be93687618e4b132087f430a4d8fc3a609c9b77c', 'a5391128b0ef5d21df5dd23d98557f4ef12fae20')
+          )
+          change_access.instance_variable_set(:@commits, project.repository.new_commits)
+
+          expect(project.repository.new_commits.first).not_to receive(:raw_deltas).and_call_original
+          expect(project.repository.new_commits.last).to receive(:raw_deltas).and_call_original
+
+          subject.validate!
+        end
+      end
+    end
   end
 end
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index a417ef77c9e..8d8eb50ad76 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -709,10 +709,22 @@ describe Gitlab::GitAccess do
       project.add_developer(user)
     end
 
-    it 'checks LFS integrity only for first change' do
-      expect_any_instance_of(Gitlab::Checks::LfsIntegrity).to receive(:objects_missing?).exactly(1).times
+    context 'when LFS is not enabled' do
+      it 'does not run LFSIntegrity check' do
+        expect(Gitlab::Checks::LfsIntegrity).not_to receive(:new)
 
-      push_access_check
+        push_access_check
+      end
+    end
+
+    context 'when LFS is enabled' do
+      it 'checks LFS integrity only for first change' do
+        allow(project).to receive(:lfs_enabled?).and_return(true)
+
+        expect_any_instance_of(Gitlab::Checks::LfsIntegrity).to receive(:objects_missing?).exactly(1).times
+
+        push_access_check
+      end
     end
   end