Commit 4e7c8d10 authored by Kerri Miller's avatar Kerri Miller

Add fallback option for overflowed MRDs

When a MergeRequestDiff is "overflowed" (contains more than a certain
number of files, 1,000 as of 12.10) #modified_paths will cut-off its
results at the last file of the limit (IE the 1,000th file.) For the
majority of MRs, this is fine, and we often have fallback code to use
Repository#diff_stats to get at the expanded list (for example, in
Gitlab::CodeOwners) This is less than ideal, as
Repository#diff_stats is slow.. like, super slow. Like, you shouldn't
use this unless you need to.

Since it is so slow, we really should have a single, unified
implementation of this fallback, and it should be here inside
MergeRequest#modifed_paths. The consumer shouldn't have to special
case their way around this limitation, and having it in 2 different
locations as of this comment, we really shouldn't have multiple places
calling their own implementations of code that is potentially very
slow. By adding this fallback option, flagged to NOT run, we can then
follow-on and remove our disparate and potentially divergent
implementations, and provide a better option for future use.
parent da275b6d
...@@ -577,13 +577,13 @@ class MergeRequest < ApplicationRecord ...@@ -577,13 +577,13 @@ class MergeRequest < ApplicationRecord
merge_request_diff&.real_size || diff_stats&.real_size || diffs.real_size merge_request_diff&.real_size || diff_stats&.real_size || diffs.real_size
end end
def modified_paths(past_merge_request_diff: nil) def modified_paths(past_merge_request_diff: nil, fallback_on_overflow: false)
if past_merge_request_diff if past_merge_request_diff
past_merge_request_diff.modified_paths past_merge_request_diff.modified_paths(fallback_on_overflow: fallback_on_overflow)
elsif compare elsif compare
diff_stats&.paths || compare.modified_paths diff_stats&.paths || compare.modified_paths
else else
merge_request_diff.modified_paths merge_request_diff.modified_paths(fallback_on_overflow: fallback_on_overflow)
end end
end end
......
...@@ -366,11 +366,24 @@ class MergeRequestDiff < ApplicationRecord ...@@ -366,11 +366,24 @@ class MergeRequestDiff < ApplicationRecord
end end
# rubocop: enable CodeReuse/ServiceClass # rubocop: enable CodeReuse/ServiceClass
def modified_paths def modified_paths(fallback_on_overflow: false)
if fallback_on_overflow && overflow?
# This is an extremely slow means to find the modified paths for a given
# MergeRequestDiff. This should be avoided, except where the limit of
# 1_000 (as of %12.10) entries returned by the default behavior is an
# issue.
strong_memoize(:overflowed_modified_paths) do
project.repository.diff_stats(
base_commit_sha,
head_commit_sha
).paths
end
else
strong_memoize(:modified_paths) do strong_memoize(:modified_paths) do
merge_request_diff_files.pluck(:new_path, :old_path).flatten.uniq merge_request_diff_files.pluck(:new_path, :old_path).flatten.uniq
end end
end end
end
# Carrierwave defines `write_uploader` dynamically on this class, so `super` # Carrierwave defines `write_uploader` dynamically on this class, so `super`
# does not work. Alias the carrierwave method so we can call it when needed # does not work. Alias the carrierwave method so we can call it when needed
......
...@@ -566,6 +566,45 @@ describe MergeRequestDiff do ...@@ -566,6 +566,45 @@ describe MergeRequestDiff do
it 'returns affected file paths' do it 'returns affected file paths' do
expect(subject.modified_paths).to eq(%w{foo bar baz}) expect(subject.modified_paths).to eq(%w{foo bar baz})
end end
context "when fallback_on_overflow is true" do
let(:merge_request) { create(:merge_request, source_branch: 'feature', target_branch: 'master') }
let(:diff) { merge_request.merge_request_diff }
# before do
# # Temporarily unstub diff.modified_paths in favor of original code
# #
# allow(diff).to receive(:modified_paths).and_call_original
# end
context "when the merge_request_diff is overflowed" do
before do
expect(diff).to receive(:overflow?).and_return(true)
end
it "returns file paths via project.repository#diff_stats" do
expect(diff.project.repository).to receive(:diff_stats).and_call_original
expect(
diff.modified_paths(fallback_on_overflow: true)
).to eq(diff.modified_paths)
end
end
context "when the merge_request_diff is not overflowed" do
before do
expect(subject).to receive(:overflow?).and_return(false)
end
it "returns expect file paths withoout called #modified_paths_for_overflowed_mr" do
expect(subject.project.repository).not_to receive(:diff_stats)
expect(
subject.modified_paths(fallback_on_overflow: true)
).to eq(subject.modified_paths)
end
end
end
end end
describe '#opening_external_diff' do describe '#opening_external_diff' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment