Merge branch 'ee-jej/avoid-slow-file-lock-checks' into 'master'

[EE MR] Avoid slow File Lock checks when not used

See merge request gitlab-org/gitlab-ee!4548

ee/lib/ee/gitlab/checks/change_access.rb

@@ -57,7 +57,7 @@ module EE
 
           commit_validation = push_rule.try(:commit_validation?)
           # if newrev is blank, the branch was deleted
-          return if deletion? || !(commit_validation || validate_path_locks?)
+          return if deletion? || !commit_validation
 
           commits.each do |commit|
             push_rule_commit_check(commit)
@@ -144,6 +144,19 @@ module EE
           end
         end
 
+        override :should_run_commit_validations?
+        def should_run_commit_validations?
+          super || validate_path_locks? || push_rule_checks_commit?
+        end
+
+        def push_rule_checks_commit?
+          return false unless push_rule
+
+          push_rule.max_file_size > 0 ||
+            push_rule.file_name_regex.present? ||
+            push_rule.prevent_secrets
+        end
+
         override :validations_for_commit
         def validations_for_commit(commit)
           validations = super

ee/spec/lib/gitlab/checks/change_access_spec.rb

@@ -236,7 +236,7 @@ describe Gitlab::Checks::ChangeAccess do
               old_rev = new_rev if new_rev
               new_rev = project.repository.create_file(user, file_path, "commit #{file_path}", message: "commit #{file_path}", branch_name: "master")
 
-              allow(project.repository).to receive(:new_commits).and_return(
+              allow(subject).to receive(:commits).and_return(
                 project.repository.commits_between(old_rev, new_rev)
               )
 

lib/gitlab/checks/change_access.rb

@@ -123,6 +123,7 @@ module Gitlab
 
       def commits_check
         return if deletion? || newrev.nil?
+        return unless should_run_commit_validations?
 
         # n+1: https://gitlab.com/gitlab-org/gitlab-ee/issues/3593
         ::Gitlab::GitalyClient.allow_n_plus_1_calls do
@@ -141,6 +142,10 @@ module Gitlab
 
       private
 
+      def should_run_commit_validations?
+        commit_check.validate_lfs_file_locks?
+      end
+
       def updated_from_web?
         protocol == 'web'
       end
@@ -178,7 +183,7 @@ module Gitlab
       end
 
       def commits
-        project.repository.new_commits(newrev)
+        @commits ||= project.repository.new_commits(newrev)
       end
     end
   end

lib/gitlab/checks/commit_check.rb

@@ -35,14 +35,14 @@ module Gitlab
         end
       end
 
-      private
-
       def validate_lfs_file_locks?
         strong_memoize(:validate_lfs_file_locks) do
           project.lfs_enabled? && project.lfs_file_locks.any? && newrev && oldrev
         end
       end
 
+      private
+
       def lfs_file_locks_validation
         lambda do |paths|
           lfs_lock = project.lfs_file_locks.where(path: paths).where.not(user_id: user.id).first

spec/lib/gitlab/checks/change_access_spec.rb

@@ -260,7 +260,7 @@ describe Gitlab::Checks::ChangeAccess do
 
       context 'with LFS not enabled' do
         it 'skips the validation' do
-          expect_any_instance_of(described_class).not_to receive(:lfs_file_locks_validation)
+          expect_any_instance_of(Gitlab::Checks::CommitCheck).not_to receive(:validate)
 
           subject.exec
         end
@@ -277,7 +277,7 @@ describe Gitlab::Checks::ChangeAccess do
           end
         end
 
-        context 'when change is sent by the author od the lock' do
+        context 'when change is sent by the author of the lock' do
           let(:user) { owner }
 
           it "doesn't raise any error" do