Merge branch 'id-improve-modified-paths-performance' into 'master'

Use diff-stats for calculating raw diffs modified paths

See merge request gitlab-org/gitlab!29134

app/models/merge_request.rb

@@ -555,22 +555,28 @@ class MergeRequest < ApplicationRecord
     end
   end
 
+  def diff_stats
+    return unless diff_refs
+
+    strong_memoize(:diff_stats) do
+      project.repository.diff_stats(diff_refs.base_sha, diff_refs.head_sha)
+    end
+  end
+
   def diff_size
     # Calling `merge_request_diff.diffs.real_size` will also perform
     # highlighting, which we don't need here.
-    merge_request_diff&.real_size || diffs.real_size
+    merge_request_diff&.real_size || diff_stats&.real_size || diffs.real_size
   end
 
   def modified_paths(past_merge_request_diff: nil)
-    diffs = if past_merge_request_diff
-              past_merge_request_diff
-            elsif compare
-              compare
-            else
-              self.merge_request_diff
-            end
-
-    diffs.modified_paths
+    if past_merge_request_diff
+      past_merge_request_diff.modified_paths
+    elsif compare
+      diff_stats&.paths || compare.modified_paths
+    else
+      merge_request_diff.modified_paths
+    end
   end
 
   def new_paths

changelogs/unreleased/id-improve-modified-paths-performance.yml

+---
+title: Use diff-stats for calculating raw diffs modified paths
+merge_request: 29134
+author:
+type: performance

lib/gitlab/git/diff_stats_collection.rb

@@ -22,6 +22,15 @@ module Gitlab
         @collection.map(&:path)
       end
 
+      def real_size
+        max_files = ::Commit.max_diff_options[:max_files]
+        if paths.size > max_files
+          "#{max_files}+"
+        else
+          paths.size.to_s
+        end
+      end
+
       private
 
       def indexed_by_path

spec/lib/gitlab/git/diff_stats_collection_spec.rb

@@ -29,4 +29,16 @@ describe Gitlab::Git::DiffStatsCollection do
       expect(collection.paths).to eq %w[foo bar]
     end
   end
+
+  describe '#real_size' do
+    it 'returns the number of modified files' do
+      expect(collection.real_size).to eq('2')
+    end
+
+    it 'returns capped number when it is bigger than max_files' do
+      allow(::Commit).to receive(:max_diff_options).and_return(max_files: 1)
+
+      expect(collection.real_size).to eq('1+')
+    end
+  end
 end

spec/models/merge_request_spec.rb

@@ -874,7 +874,7 @@ describe MergeRequest do
     subject(:merge_request) { build(:merge_request) }
 
     before do
-      expect(diff).to receive(:modified_paths).and_return(paths)
+      allow(diff).to receive(:modified_paths).and_return(paths)
     end
 
     context 'when past_merge_request_diff is specified' do
@@ -890,10 +890,29 @@ describe MergeRequest do
       let(:compare) { double(:compare) }
       let(:diff) { compare }
 
-      it 'returns affected file paths from compare' do
+      before do
         merge_request.compare = compare
 
-        expect(merge_request.modified_paths).to eq(paths)
+        expect(merge_request).to receive(:diff_stats).and_return(diff_stats)
+      end
+
+      context 'and diff_stats are not present' do
+        let(:diff_stats) { nil }
+
+        it 'returns affected file paths from compare' do
+          expect(merge_request.modified_paths).to eq(paths)
+        end
+      end
+
+      context 'and diff_stats are present' do
+        let(:diff_stats) { double(:diff_stats) }
+
+        it 'returns affected file paths from compare' do
+          diff_stats_path = double(:diff_stats_paths)
+          expect(diff_stats).to receive(:paths).and_return(diff_stats_path)
+
+          expect(merge_request.modified_paths).to eq(diff_stats_path)
+        end
       end
     end