Merge branch '227572-n-1-commitdelta-for-large-pushes-2' into 'master'

Resolve "N + 1: CommitDelta for large pushes"

See merge request gitlab-org/gitlab!46116

Gemfile

@@ -466,7 +466,7 @@ group :ed25519 do
 end
 
 # Gitaly GRPC protocol definitions
-gem 'gitaly', '~> 13.6.1'
+gem 'gitaly', '~> 13.7.0.pre.rc1'
 
 gem 'grpc', '~> 1.30.2'
 

Gemfile.lock

@@ -422,7 +422,7 @@ GEM
       rails (>= 3.2.0)
     git (1.7.0)
       rchardet (~> 1.8)
-    gitaly (13.6.1)
+    gitaly (13.7.0.pre.rc1)
       grpc (~> 1.0)
     github-markup (1.7.0)
     gitlab-chronic (0.10.5)
@@ -1349,7 +1349,7 @@ DEPENDENCIES
   gettext (~> 3.3)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.3)
-  gitaly (~> 13.6.1)
+  gitaly (~> 13.7.0.pre.rc1)
   github-markup (~> 1.7.0)
   gitlab-chronic (~> 0.10.5)
   gitlab-fog-azure-rm (~> 1.0)

config/feature_flags/development/diff_check_with_paths_changed_rpc.yml

+---
+name: diff_check_with_paths_changed_rpc
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/46116
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/288827
+milestone: '13.7'
+type: development
+group: group::code review
+default_enabled: false

ee/lib/ee/gitlab/checks/diff_check.rb

@@ -58,11 +58,15 @@ module EE
 
         def path_locks_validation
           lambda do |diff|
-            path = if diff.renamed_file?
+            path = if ::Feature.enabled?(:diff_check_with_paths_changed_rpc, project)
+                     diff.path
+                   else
+                     if diff.renamed_file?
                        diff.old_path
                      else
                        diff.new_path || diff.old_path
                      end
+                   end
 
             lock_info = project.find_path_lock(path)
 
@@ -72,12 +76,24 @@ module EE
           end
         end
 
+        def new_file?(path)
+          path.status == :ADDED
+        end
+
         def file_name_validation
           lambda do |diff|
-            if (diff.renamed_file || diff.new_file) && blacklisted_regex = push_rule.filename_denylisted?(diff.new_path)
-              return unless blacklisted_regex.present?
+            if ::Feature.enabled?(:diff_check_with_paths_changed_rpc, project)
+              if new_file?(diff) && denylisted_regex = push_rule.filename_denylisted?(diff.path)
+                return unless denylisted_regex.present?
 
-              "File name #{diff.new_path} was blacklisted by the pattern #{blacklisted_regex}."
+                "File name #{diff.path} was blacklisted by the pattern #{denylisted_regex}."
+              end
+            else
+              if (diff.renamed_file || diff.new_file) && denylisted_regex = push_rule.filename_denylisted?(diff.new_path)
+                return unless denylisted_regex.present?
+
+                "File name #{diff.new_path} was blacklisted by the pattern #{denylisted_regex}."
+              end
             end
           rescue ::PushRule::MatchError => e
             raise ::Gitlab::GitAccess::ForbiddenError, e.message

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

@@ -5,6 +5,7 @@ require 'spec_helper'
 RSpec.describe Gitlab::Checks::DiffCheck do
   include FakeBlobHelpers
 
+  shared_examples_for "diff check" do
     include_context 'push rules checks context'
 
     describe '#validate!' do
@@ -36,6 +37,18 @@ RSpec.describe Gitlab::Checks::DiffCheck do
         let(:protocol) { 'ssh' }
         let(:push_allowed) { false }
 
+        context 'when push_rules_supersede_code_owners is disabled' do
+          before do
+            stub_feature_flags(push_rules_supersede_code_owners: false)
+          end
+
+          it 'returns branch_requires_code_owner_approval?' do
+            expect(project).to receive(:branch_requires_code_owner_approval?).and_return(true)
+
+            expect(validate_code_owners).to eq(true)
+          end
+        end
+
         context 'when user can not push to the branch' do
           context 'when not updated from web' do
             it 'checks if the branch requires code owner approval' do
@@ -60,18 +73,6 @@ RSpec.describe Gitlab::Checks::DiffCheck do
           it 'returns false' do
             expect(validate_code_owners).to eq(false)
           end
-
-        context 'when push_rules_supersede_code_owners is disabled' do
-          before do
-            stub_feature_flags(push_rules_supersede_code_owners: false)
-          end
-
-          it 'returns branch_requires_code_owner_approval?' do
-            expect(project).to receive(:branch_requires_code_owner_approval?).and_return(true)
-
-            expect(validate_code_owners).to eq(true)
-          end
-        end
         end
       end
 
@@ -338,4 +339,15 @@ RSpec.describe Gitlab::Checks::DiffCheck do
         end
       end
     end
+  end
+
+  it_behaves_like "diff check"
+
+  context 'when diff check with paths rpc feature flag is false' do
+    before do
+      stub_feature_flags(diff_check_with_paths_changed_rpc: false)
+    end
+
+    it_behaves_like "diff check"
+  end
 end

lib/gitlab/checks/diff_check.rb

@@ -17,6 +17,14 @@ module Gitlab
 
         file_paths = []
 
+        if ::Feature.enabled?(:diff_check_with_paths_changed_rpc, project)
+          paths = project.repository.find_changed_paths(commits.map(&:sha))
+          paths.each do |path|
+            file_paths.concat([path.path])
+
+            validate_diff(path)
+          end
+        else
           process_commits do |commit|
             validate_once(commit) do
               commit.raw_deltas.each do |diff|
@@ -26,8 +34,9 @@ module Gitlab
               end
             end
           end
+        end
 
-        validate_file_paths(file_paths)
+        validate_file_paths(file_paths.uniq)
       end
 
       private

lib/gitlab/git/repository.rb

@@ -467,6 +467,18 @@ module Gitlab
         empty_diff_stats
       end
 
+      def find_changed_paths(commits)
+        processed_commits = commits.reject { |ref| ref.blank? || Gitlab::Git.blank_ref?(ref) }
+
+        return [] if processed_commits.empty?
+
+        wrapped_gitaly_errors do
+          gitaly_commit_client.find_changed_paths(processed_commits)
+        end
+      rescue CommandError, TypeError, NoRepository
+        []
+      end
+
       # Returns a RefName for a given SHA
       def ref_name_for_sha(ref_path, sha)
         raise ArgumentError, "sha can't be empty" unless sha.present?

lib/gitlab/gitaly_client/commit_service.rb

@@ -216,6 +216,23 @@ module Gitlab
         response.flat_map(&:stats)
       end
 
+      def find_changed_paths(commits)
+        request = Gitaly::FindChangedPathsRequest.new(
+          repository: @gitaly_repo,
+          commits: commits
+        )
+
+        response = GitalyClient.call(@repository.storage, :diff_service, :find_changed_paths, request, timeout: GitalyClient.medium_timeout)
+        response.flat_map do |msg|
+          msg.paths.map do |path|
+            OpenStruct.new(
+              status: path.status,
+              path:  EncodingHelper.encode!(path.path)
+            )
+          end
+        end
+      end
+
       def find_all_commits(opts = {})
         request = Gitaly::FindAllCommitsRequest.new(
           repository: @gitaly_repo,

spec/lib/gitlab/checks/diff_check_spec.rb

@@ -7,7 +7,6 @@ RSpec.describe Gitlab::Checks::DiffCheck do
 
   describe '#validate!' do
     let(:owner) { create(:user) }
-    let!(:lock) { create(:lfs_file_lock, user: owner, project: project, path: 'README') }
 
     before do
       allow(project.repository).to receive(:new_commits).and_return(
@@ -28,16 +27,30 @@ RSpec.describe Gitlab::Checks::DiffCheck do
     end
 
     context 'with LFS enabled' do
+      let!(:lock) { create(:lfs_file_lock, user: owner, project: project, path: 'README') }
+
       before do
         allow(project).to receive(:lfs_enabled?).and_return(true)
       end
 
       context 'when change is sent by a different user' do
+        context 'when diff check with paths rpc feature flag is true' do
           it 'raises an error if the user is not allowed to update the file' do
             expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, "The path 'README' is locked in Git LFS by #{lock.user.name}")
           end
         end
 
+        context 'when diff check with paths rpc feature flag is false' do
+          before do
+            stub_feature_flags(diff_check_with_paths_changed_rpc: false)
+          end
+
+          it 'raises an error if the user is not allowed to update the file' do
+            expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, "The path 'README' is locked in Git LFS by #{lock.user.name}")
+          end
+        end
+      end
+
       context 'when change is sent by the author of the lock' do
         let(:user) { owner }
 
@@ -53,6 +66,8 @@ RSpec.describe Gitlab::Checks::DiffCheck do
 
         expect_any_instance_of(Commit).to receive(:raw_deltas).and_call_original
 
+        stub_feature_flags(diff_check_with_paths_changed_rpc: false)
+
         subject.validate!
       end
 

spec/lib/gitlab/git/repository_spec.rb

@@ -1185,6 +1185,66 @@ RSpec.describe Gitlab::Git::Repository, :seed_helper do
     end
   end
 
+  describe '#find_changed_paths' do
+    let(:commit_1) { 'fa1b1e6c004a68b7d8763b86455da9e6b23e36d6' }
+    let(:commit_2) { '4b4918a572fa86f9771e5ba40fbd48e1eb03e2c6' }
+    let(:commit_3) { '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9' }
+    let(:commit_1_files) do
+      [
+        OpenStruct.new(status: :ADDED, path: "files/executables/ls"),
+        OpenStruct.new(status: :ADDED, path: "files/executables/touch"),
+        OpenStruct.new(status: :ADDED, path: "files/links/regex.rb"),
+        OpenStruct.new(status: :ADDED, path: "files/links/ruby-style-guide.md"),
+        OpenStruct.new(status: :ADDED, path: "files/links/touch"),
+        OpenStruct.new(status: :MODIFIED, path: ".gitmodules"),
+        OpenStruct.new(status: :ADDED, path: "deeper/nested/six"),
+        OpenStruct.new(status: :ADDED, path: "nested/six")
+      ]
+    end
+
+    let(:commit_2_files) do
+      [OpenStruct.new(status: :ADDED, path: "bin/executable")]
+    end
+
+    let(:commit_3_files) do
+      [
+        OpenStruct.new(status: :MODIFIED, path: ".gitmodules"),
+        OpenStruct.new(status: :ADDED, path: "gitlab-shell")
+      ]
+    end
+
+    it 'returns a list of paths' do
+      collection = repository.find_changed_paths([commit_1, commit_2, commit_3])
+
+      expect(collection).to be_a(Enumerable)
+      expect(collection.to_a).to eq(commit_1_files + commit_2_files + commit_3_files)
+    end
+
+    it 'returns no paths when SHAs are invalid' do
+      collection = repository.find_changed_paths(['invalid', commit_1])
+
+      expect(collection).to be_a(Enumerable)
+      expect(collection.to_a).to be_empty
+    end
+
+    it 'returns a list of paths even when containing a blank ref' do
+      collection = repository.find_changed_paths([nil, commit_1])
+
+      expect(collection).to be_a(Enumerable)
+      expect(collection.to_a).to eq(commit_1_files)
+    end
+
+    it 'returns no paths when the commits are nil' do
+      expect_any_instance_of(Gitlab::GitalyClient::CommitService)
+        .not_to receive(:find_changed_paths)
+
+      collection = repository.find_changed_paths([nil, nil])
+
+      expect(collection).to be_a(Enumerable)
+      expect(collection.to_a).to be_empty
+    end
+  end
+
   describe "#ls_files" do
     let(:master_file_paths) { repository.ls_files("master") }
     let(:utf8_file_paths) { repository.ls_files("ls-files-utf8") }

spec/lib/gitlab/gitaly_client/commit_service_spec.rb

@@ -145,6 +145,31 @@ RSpec.describe Gitlab::GitalyClient::CommitService do
     end
   end
 
+  describe '#find_changed_paths' do
+    let(:commits) { %w[1a0b36b3cdad1d2ee32457c102a8c0b7056fa863 cfe32cf61b73a0d5e9f13e774abde7ff789b1660] }
+
+    it 'sends an RPC request and returns the stats' do
+      request = Gitaly::FindChangedPathsRequest.new(repository: repository_message,
+                                                    commits: commits)
+
+      changed_paths_response = Gitaly::FindChangedPathsResponse.new(
+        paths: [{
+          path: "app/assets/javascripts/boards/components/project_select.vue",
+          status: :MODIFIED
+        }])
+
+      expect_any_instance_of(Gitaly::DiffService::Stub).to receive(:find_changed_paths)
+        .with(request, kind_of(Hash)).and_return([changed_paths_response])
+
+      returned_value = described_class.new(repository).find_changed_paths(commits)
+
+      mapped_returned_value = returned_value.map(&:to_h)
+      mapped_expected_value = changed_paths_response.paths.map(&:to_h)
+
+      expect(mapped_returned_value).to eq(mapped_expected_value)
+    end
+  end
+
   describe '#tree_entries' do
     let(:path) { '/' }