diff options
| -rw-r--r-- | app/models/commit.rb | 4 | ||||
| -rw-r--r-- | changelogs/unreleased/fj-44679-skip-per-commit-validations.yml | 5 | ||||
| -rw-r--r-- | lib/gitlab/checks/base_checker.rb | 16 | ||||
| -rw-r--r-- | lib/gitlab/checks/diff_check.rb | 24 | ||||
| -rw-r--r-- | lib/gitlab/checks/lfs_check.rb | 1 | ||||
| -rw-r--r-- | spec/lib/gitlab/checks/diff_check_spec.rb | 38 | ||||
| -rw-r--r-- | spec/lib/gitlab/git_access_spec.rb | 18 |
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 |