From 092b92b1afd2abb7ff10f56616eea299bb572e5a Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Wed, 7 Oct 2020 11:28:13 -0700 Subject: [PATCH 01/15] Extract #expand_diff? method --- lib/gitlab/diff/highlight_cache.rb | 3 +++ lib/gitlab/git/diff_collection.rb | 12 +++++++----- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb index e873e9c17d57cb..f6f1b78ebd4b09 100644 --- a/lib/gitlab/diff/highlight_cache.rb +++ b/lib/gitlab/diff/highlight_cache.rb @@ -24,6 +24,9 @@ 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 + # 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 2771057f51b876..3d3e6e1dffcd4c 100644 --- a/lib/gitlab/git/diff_collection.rb +++ b/lib/gitlab/git/diff_collection.rb @@ -118,11 +118,15 @@ def over_safe_limits?(files) files >= safe_max_files || @line_count > safe_max_lines || @byte_count >= safe_max_bytes end + def expand_diff? + !@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 +149,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 -- GitLab From f395a84b16f6bf13108678fce445661c65dd7875 Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Wed, 7 Oct 2020 12:15:00 -0700 Subject: [PATCH 02/15] Return true if collection only contains 1 item --- lib/gitlab/git/diff_collection.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/gitlab/git/diff_collection.rb b/lib/gitlab/git/diff_collection.rb index 3d3e6e1dffcd4c..d2e006012c329c 100644 --- a/lib/gitlab/git/diff_collection.rb +++ b/lib/gitlab/git/diff_collection.rb @@ -119,6 +119,10 @@ def over_safe_limits?(files) end def expand_diff? + # Force single-entry diff collections to always present as expanded + # + return true if @iterator.length == 1 + !@enforce_limits || @expanded end -- GitLab From b77c24a0cec4620f1f7b3c835d384a9398ba0b27 Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Wed, 7 Oct 2020 13:00:49 -0700 Subject: [PATCH 03/15] Prefer #size > #length for Enumerables --- lib/gitlab/git/diff_collection.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab/git/diff_collection.rb b/lib/gitlab/git/diff_collection.rb index d2e006012c329c..11a7f749f07a9c 100644 --- a/lib/gitlab/git/diff_collection.rb +++ b/lib/gitlab/git/diff_collection.rb @@ -121,7 +121,7 @@ def over_safe_limits?(files) def expand_diff? # Force single-entry diff collections to always present as expanded # - return true if @iterator.length == 1 + return true if @iterator.size == 1 !@enforce_limits || @expanded end -- GitLab From 0ec01182c953dfe1b3f01af2a0cfbce902612be0 Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Wed, 7 Oct 2020 13:01:20 -0700 Subject: [PATCH 04/15] Allow testing iterator to report its size --- spec/lib/gitlab/git/diff_collection_spec.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/spec/lib/gitlab/git/diff_collection_spec.rb b/spec/lib/gitlab/git/diff_collection_spec.rb index 8198c2651a72e5..843965f6312a50 100644 --- a/spec/lib/gitlab/git/diff_collection_spec.rb +++ b/spec/lib/gitlab/git/diff_collection_spec.rb @@ -11,9 +11,12 @@ def initialize(count, value) @count = count + @size = count @value = value end + attr_reader :size + def each return enum_for(:each) unless block_given? -- GitLab From 444b2e94aa4e40b213b36465c3c5824e02cef17c Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Wed, 7 Oct 2020 13:52:32 -0700 Subject: [PATCH 05/15] Update test to account for forced expansion --- spec/lib/gitlab/git/diff_collection_spec.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spec/lib/gitlab/git/diff_collection_spec.rb b/spec/lib/gitlab/git/diff_collection_spec.rb index 843965f6312a50..41826793b6c7fd 100644 --- a/spec/lib/gitlab/git/diff_collection_spec.rb +++ b/spec/lib/gitlab/git/diff_collection_spec.rb @@ -521,6 +521,8 @@ def each end it 'prunes diffs that are quite big' do + allow(subject).to receive(:expand_diff?).and_return(expanded) + diff = nil subject.each do |d| -- GitLab From 579d6880bdef0f3601ecd56ffe232549cdb72044 Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Wed, 7 Oct 2020 14:03:13 -0700 Subject: [PATCH 06/15] Build single- vs multi-file tests --- spec/lib/gitlab/git/diff_collection_spec.rb | 26 ++++++++++++++++----- 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/spec/lib/gitlab/git/diff_collection_spec.rb b/spec/lib/gitlab/git/diff_collection_spec.rb index 41826793b6c7fd..71d08fd7facdbe 100644 --- a/spec/lib/gitlab/git/diff_collection_spec.rb +++ b/spec/lib/gitlab/git/diff_collection_spec.rb @@ -520,16 +520,30 @@ def each .to yield_with_args(an_instance_of(Gitlab::Git::Diff)) end - it 'prunes diffs that are quite big' do - allow(subject).to receive(:expand_diff?).and_return(expanded) + context 'single-file collections' do + it 'does not prune diffs' do + diff = nil - diff = nil + subject.each do |d| + diff = d + end - subject.each do |d| - diff = d + expect(diff.diff).not_to eq('') end + end - expect(diff.diff).to eq('') + context 'multi-file collections' do + let(:iterator) { [{ diff: 'b' }, { diff: 'a' * 20480 }]} + + it 'prunes diffs that are quite big' do + diff = nil + + subject.each do |d| + diff = d + end + + expect(diff.diff).to eq('') + end end context 'when go over safe limits on files' do -- GitLab From 629346fc90e0ed2a4ec0e9092e0a69c3f3bbc410 Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Wed, 7 Oct 2020 15:23:25 -0700 Subject: [PATCH 07/15] Enable DiffStitcher to report Enumerator size --- lib/gitlab/gitaly_client/diff_stitcher.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/gitlab/gitaly_client/diff_stitcher.rb b/lib/gitlab/gitaly_client/diff_stitcher.rb index 98d327a732918d..d73bed6fd772cf 100644 --- a/lib/gitlab/gitaly_client/diff_stitcher.rb +++ b/lib/gitlab/gitaly_client/diff_stitcher.rb @@ -9,6 +9,10 @@ def initialize(rpc_response) @rpc_response = rpc_response end + def size + @rpc_response.size + end + def each current_diff = nil -- GitLab From 4726d43b397684767d7b1c3e572b8a3af5962411 Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Wed, 7 Oct 2020 16:40:17 -0700 Subject: [PATCH 08/15] Add changelog to MR --- ...-expand-file-on-merge-request-with-changes-to-single-.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/254823-automatically-expand-file-on-merge-request-with-changes-to-single-.yml 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 00000000000000..42a1d7c68478ee --- /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 -- GitLab From 93dba36c0f5672e79fbe57f6c310ce720384b82c Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Thu, 8 Oct 2020 11:07:07 -0700 Subject: [PATCH 09/15] Update comment with issue link --- lib/gitlab/diff/highlight_cache.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb index f6f1b78ebd4b09..90cb9c8638a8bd 100644 --- a/lib/gitlab/diff/highlight_cache.rb +++ b/lib/gitlab/diff/highlight_cache.rb @@ -26,6 +26,7 @@ def decorate(diff_file) # 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 -- GitLab From 3a95bfd6657622e032da291c7d83386772601620 Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Thu, 15 Oct 2020 06:01:56 +0000 Subject: [PATCH 10/15] Apply 1 suggestion(s) to 1 file(s) --- lib/gitlab/gitaly_client/diff_stitcher.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/gitlab/gitaly_client/diff_stitcher.rb b/lib/gitlab/gitaly_client/diff_stitcher.rb index d73bed6fd772cf..285b1f63e27e24 100644 --- a/lib/gitlab/gitaly_client/diff_stitcher.rb +++ b/lib/gitlab/gitaly_client/diff_stitcher.rb @@ -9,10 +9,11 @@ def initialize(rpc_response) @rpc_response = rpc_response end - def size - @rpc_response.size - end + delegate :size, to: :rpc_response + + private + attr_reader :rpc_response def each current_diff = nil -- GitLab From e04aaf775309091b1065b8fbfbdeba581af3e19a Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Thu, 15 Oct 2020 06:02:01 +0000 Subject: [PATCH 11/15] Apply 1 suggestion(s) to 1 file(s) --- lib/gitlab/git/diff_collection.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/gitlab/git/diff_collection.rb b/lib/gitlab/git/diff_collection.rb index 11a7f749f07a9c..6090d1b9f69ee3 100644 --- a/lib/gitlab/git/diff_collection.rb +++ b/lib/gitlab/git/diff_collection.rb @@ -121,9 +121,7 @@ def over_safe_limits?(files) def expand_diff? # Force single-entry diff collections to always present as expanded # - return true if @iterator.size == 1 - - !@enforce_limits || @expanded + @iterator.size == 1 || !@enforce_limits || @expanded end def each_gitaly_patch -- GitLab From d90546cee660336c3fd0765510ca1cb6a98f66f7 Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Thu, 15 Oct 2020 04:12:03 -0700 Subject: [PATCH 12/15] Move delegate to top of class --- lib/gitlab/gitaly_client/diff_stitcher.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/gitaly_client/diff_stitcher.rb b/lib/gitlab/gitaly_client/diff_stitcher.rb index 285b1f63e27e24..56019d0990edac 100644 --- a/lib/gitlab/gitaly_client/diff_stitcher.rb +++ b/lib/gitlab/gitaly_client/diff_stitcher.rb @@ -5,12 +5,12 @@ module GitalyClient class DiffStitcher include Enumerable + delegate :size, to: :rpc_response + def initialize(rpc_response) @rpc_response = rpc_response end - delegate :size, to: :rpc_response - private attr_reader :rpc_response -- GitLab From 23a22faa587a7b95dfb4d7a8ef1e6613ea9b1f37 Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Thu, 15 Oct 2020 04:13:49 -0700 Subject: [PATCH 13/15] Relocate private declaration --- lib/gitlab/gitaly_client/diff_stitcher.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/gitlab/gitaly_client/diff_stitcher.rb b/lib/gitlab/gitaly_client/diff_stitcher.rb index 56019d0990edac..9b22102a379baa 100644 --- a/lib/gitlab/gitaly_client/diff_stitcher.rb +++ b/lib/gitlab/gitaly_client/diff_stitcher.rb @@ -11,9 +11,6 @@ def initialize(rpc_response) @rpc_response = rpc_response end - private - - attr_reader :rpc_response def each current_diff = nil @@ -36,6 +33,10 @@ def each end end end + + private + + attr_reader :rpc_response end end end -- GitLab From 1254c4796a4701a905fd7345b1c187ee9e8ea80f Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Thu, 15 Oct 2020 04:14:10 -0700 Subject: [PATCH 14/15] Rename param to avoid method shadowing --- lib/gitlab/gitaly_client/diff_stitcher.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/gitaly_client/diff_stitcher.rb b/lib/gitlab/gitaly_client/diff_stitcher.rb index 9b22102a379baa..e98ae75590d9ff 100644 --- a/lib/gitlab/gitaly_client/diff_stitcher.rb +++ b/lib/gitlab/gitaly_client/diff_stitcher.rb @@ -7,8 +7,8 @@ class DiffStitcher delegate :size, to: :rpc_response - def initialize(rpc_response) - @rpc_response = rpc_response + def initialize(rpc_response_param) + @rpc_response = rpc_response_param end def each -- GitLab From 0699883fab5dd3bc2deec2e9d166d809284e52d6 Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Thu, 15 Oct 2020 04:19:58 -0700 Subject: [PATCH 15/15] Relocated attr_reader declaration --- spec/lib/gitlab/git/diff_collection_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/lib/gitlab/git/diff_collection_spec.rb b/spec/lib/gitlab/git/diff_collection_spec.rb index 71d08fd7facdbe..1a3c332a21bc5b 100644 --- a/spec/lib/gitlab/git/diff_collection_spec.rb +++ b/spec/lib/gitlab/git/diff_collection_spec.rb @@ -9,14 +9,14 @@ MutatingConstantIterator.class_eval do include Enumerable + attr_reader :size + def initialize(count, value) @count = count @size = count @value = value end - attr_reader :size - def each return enum_for(:each) unless block_given? -- GitLab