diff --git a/changelogs/unreleased/254823-automatically-expand-file-on-merge-request-with-changes-to-single-.yml b/changelogs/unreleased/254823-automatically-expand-file-on-merge-request-with-changes-to-single-.yml new file mode 100644 index 0000000000000000000000000000000000000000..42a1d7c68478ee1929803d25bc22af6a526e8d38 --- /dev/null +++ b/changelogs/unreleased/254823-automatically-expand-file-on-merge-request-with-changes-to-single-.yml @@ -0,0 +1,5 @@ +--- +title: Automatically expand diffs for merge requests with changes to a single file +merge_request: 44629 +author: +type: changed diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb index e873e9c17d57cbe5af03f66ad607a974042345bf..90cb9c8638a8bdd850f8aede1509b55bad81f62b 100644 --- a/lib/gitlab/diff/highlight_cache.rb +++ b/lib/gitlab/diff/highlight_cache.rb @@ -24,6 +24,10 @@ def decorate(diff_file) return [] unless content + # TODO: We could add some kind of flag to #initialize that would allow + # us to force re-caching + # https://gitlab.com/gitlab-org/gitlab/-/issues/263508 + # if content.empty? && recache_due_to_size?(diff_file) # If the file is missing from the cache and there's reason to believe # it is uncached due to a size issue around changing the values for diff --git a/lib/gitlab/git/diff_collection.rb b/lib/gitlab/git/diff_collection.rb index 2771057f51b876c6959f756da7ec450945fef947..6090d1b9f69ee3559c9ca28cf35ba08dd8d74c19 100644 --- a/lib/gitlab/git/diff_collection.rb +++ b/lib/gitlab/git/diff_collection.rb @@ -118,11 +118,17 @@ def over_safe_limits?(files) files >= safe_max_files || @line_count > safe_max_lines || @byte_count >= safe_max_bytes end + def expand_diff? + # Force single-entry diff collections to always present as expanded + # + @iterator.size == 1 || !@enforce_limits || @expanded + end + def each_gitaly_patch i = @array.length @iterator.each do |raw| - diff = Gitlab::Git::Diff.new(raw, expanded: !@enforce_limits || @expanded) + diff = Gitlab::Git::Diff.new(raw, expanded: expand_diff?) if raw.overflow_marker @overflow = true @@ -145,11 +151,9 @@ def each_serialized_patch break end - expanded = !@enforce_limits || @expanded - - diff = Gitlab::Git::Diff.new(raw, expanded: expanded) + diff = Gitlab::Git::Diff.new(raw, expanded: expand_diff?) - if !expanded && over_safe_limits?(i) && diff.line_count > 0 + if !expand_diff? && over_safe_limits?(i) && diff.line_count > 0 diff.collapse! end diff --git a/lib/gitlab/gitaly_client/diff_stitcher.rb b/lib/gitlab/gitaly_client/diff_stitcher.rb index 98d327a732918db81f19f1eeaa734c74f084fc3e..e98ae75590d9ff48cb3c621478c9e19a05530f17 100644 --- a/lib/gitlab/gitaly_client/diff_stitcher.rb +++ b/lib/gitlab/gitaly_client/diff_stitcher.rb @@ -5,8 +5,10 @@ module GitalyClient class DiffStitcher include Enumerable - def initialize(rpc_response) - @rpc_response = rpc_response + delegate :size, to: :rpc_response + + def initialize(rpc_response_param) + @rpc_response = rpc_response_param end def each @@ -31,6 +33,10 @@ def each end end end + + private + + attr_reader :rpc_response end end end diff --git a/spec/lib/gitlab/git/diff_collection_spec.rb b/spec/lib/gitlab/git/diff_collection_spec.rb index 8198c2651a72e564f0b8b8b79d30fa3e2e839387..1a3c332a21bc5b89fb50ac5351e920027f7f60ea 100644 --- a/spec/lib/gitlab/git/diff_collection_spec.rb +++ b/spec/lib/gitlab/git/diff_collection_spec.rb @@ -9,8 +9,11 @@ MutatingConstantIterator.class_eval do include Enumerable + attr_reader :size + def initialize(count, value) @count = count + @size = count @value = value end @@ -517,14 +520,30 @@ def each .to yield_with_args(an_instance_of(Gitlab::Git::Diff)) end - it 'prunes diffs that are quite big' do - diff = nil + context 'single-file collections' do + it 'does not prune diffs' do + diff = nil - subject.each do |d| - diff = d + subject.each do |d| + diff = d + end + + expect(diff.diff).not_to eq('') end + end + + context 'multi-file collections' do + let(:iterator) { [{ diff: 'b' }, { diff: 'a' * 20480 }]} + + it 'prunes diffs that are quite big' do + diff = nil - expect(diff.diff).to eq('') + subject.each do |d| + diff = d + end + + expect(diff.diff).to eq('') + end end context 'when go over safe limits on files' do