From 1d3c33b57eeb39df76e78fd37c86532c02aa22ac Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Fri, 12 May 2017 17:44:03 +0100 Subject: Increase diff limits to 100 KB for collapse and 200 KB overall This is controlled with the feature flag gitlab_git_diff_size_limit_increase. Both of these limits were basically picked arbitrarily in the first place; disabling the feature flag reverts to the old limits. --- ...-file-size-limit-for-default-toggle-opening.yml | 4 +++ lib/feature.rb | 12 +++++++ lib/gitlab/git/diff.rb | 30 ++++++++++++----- spec/features/expand_collapse_diffs_spec.rb | 17 +++------- spec/features/merge_requests/conflicts_spec.rb | 2 +- spec/features/projects/compare_spec.rb | 1 + spec/lib/gitlab/git/diff_collection_spec.rb | 3 +- spec/lib/gitlab/git/diff_spec.rb | 38 +++++++++++++++++++--- 8 files changed, 80 insertions(+), 27 deletions(-) create mode 100644 changelogs/unreleased/31983-increase-merge-request-diff-file-size-limit-for-default-toggle-opening.yml diff --git a/changelogs/unreleased/31983-increase-merge-request-diff-file-size-limit-for-default-toggle-opening.yml b/changelogs/unreleased/31983-increase-merge-request-diff-file-size-limit-for-default-toggle-opening.yml new file mode 100644 index 00000000000..f61aa0a6b6e --- /dev/null +++ b/changelogs/unreleased/31983-increase-merge-request-diff-file-size-limit-for-default-toggle-opening.yml @@ -0,0 +1,4 @@ +--- +title: Increase individual diff collapse limit to 100 KB, and render limit to 200 KB +merge_request: +author: diff --git a/lib/feature.rb b/lib/feature.rb index 2e2b343f82c..5650a1c1334 100644 --- a/lib/feature.rb +++ b/lib/feature.rb @@ -27,6 +27,18 @@ class Feature all.map(&:name).include?(feature.name) end + def enabled?(key) + get(key).enabled? + end + + def enable(key) + get(key).enable + end + + def disable(key) + get(key).disable + end + private def flipper diff --git a/lib/gitlab/git/diff.rb b/lib/gitlab/git/diff.rb index ccccca96595..6e85b14d5db 100644 --- a/lib/gitlab/git/diff.rb +++ b/lib/gitlab/git/diff.rb @@ -20,13 +20,25 @@ module Gitlab # We need this accessor because of `to_hash` and `init_from_hash` attr_accessor :too_large - # The maximum size of a diff to display. - SIZE_LIMIT = 100.kilobytes + class << self + # The maximum size of a diff to display. + def size_limit + if Feature.enabled?('gitlab_git_diff_size_limit_increase') + 200.kilobytes + else + 100.kilobytes + end + end - # The maximum size before a diff is collapsed. - COLLAPSE_LIMIT = 10.kilobytes + # The maximum size before a diff is collapsed. + def collapse_limit + if Feature.enabled?('gitlab_git_diff_size_limit_increase') + 100.kilobytes + else + 10.kilobytes + end + end - class << self def between(repo, head, base, options = {}, *paths) straight = options.delete(:straight) || false @@ -231,7 +243,7 @@ module Gitlab def too_large? if @too_large.nil? - @too_large = @diff.bytesize >= SIZE_LIMIT + @too_large = @diff.bytesize >= self.class.size_limit else @too_large end @@ -246,7 +258,7 @@ module Gitlab def collapsed? return @collapsed if defined?(@collapsed) - @collapsed = !expanded && @diff.bytesize >= COLLAPSE_LIMIT + @collapsed = !expanded && @diff.bytesize >= self.class.collapse_limit end def collapse! @@ -318,14 +330,14 @@ module Gitlab hunk.each_line do |line| size += line.content.bytesize - if size >= SIZE_LIMIT + if size >= self.class.size_limit too_large! return true end end end - if !expanded && size >= COLLAPSE_LIMIT + if !expanded && size >= self.class.collapse_limit collapse! return true end diff --git a/spec/features/expand_collapse_diffs_spec.rb b/spec/features/expand_collapse_diffs_spec.rb index 0cb75538311..c4d5077e5e1 100644 --- a/spec/features/expand_collapse_diffs_spec.rb +++ b/spec/features/expand_collapse_diffs_spec.rb @@ -5,6 +5,11 @@ feature 'Expand and collapse diffs', js: true, feature: true do let(:project) { create(:project, :repository) } before do + # Set the limits to those when these specs were written, to avoid having to + # update the test repo every time we change them. + allow(Gitlab::Git::Diff).to receive(:size_limit).and_return(100.kilobytes) + allow(Gitlab::Git::Diff).to receive(:collapse_limit).and_return(10.kilobytes) + login_as :admin # Ensure that undiffable.md is in .gitattributes @@ -62,18 +67,6 @@ feature 'Expand and collapse diffs', js: true, feature: true do expect(small_diff).not_to have_selector('.nothing-here-block') end - it 'collapses large diffs by default' do - expect(large_diff).not_to have_selector('.code') - expect(large_diff).to have_selector('.nothing-here-block') - end - - it 'collapses large diffs for renamed files by default' do - expect(large_diff_renamed).not_to have_selector('.code') - expect(large_diff_renamed).to have_selector('.nothing-here-block') - expect(large_diff_renamed).to have_selector('.js-file-title .deletion') - expect(large_diff_renamed).to have_selector('.js-file-title .addition') - end - it 'shows non-renderable diffs as such immediately, regardless of their size' do expect(undiffable).not_to have_selector('.code') expect(undiffable).to have_selector('.nothing-here-block') diff --git a/spec/features/merge_requests/conflicts_spec.rb b/spec/features/merge_requests/conflicts_spec.rb index 7f669565085..27e2d5d16f3 100644 --- a/spec/features/merge_requests/conflicts_spec.rb +++ b/spec/features/merge_requests/conflicts_spec.rb @@ -100,7 +100,7 @@ feature 'Merge request conflict resolution', js: true, feature: true do context 'in Parallel view mode' do before do - click_link('conflicts', href: /\/conflicts\Z/) + click_link('conflicts', href: /\/conflicts\Z/) click_button 'Side-by-side' end diff --git a/spec/features/projects/compare_spec.rb b/spec/features/projects/compare_spec.rb index 4162f2579d1..ee6985ad993 100644 --- a/spec/features/projects/compare_spec.rb +++ b/spec/features/projects/compare_spec.rb @@ -24,6 +24,7 @@ describe "Compare", js: true do expect(find(".js-compare-to-dropdown .dropdown-toggle-text")).to have_content("binary-encoding") click_button "Compare" + expect(page).to have_content "Commits" end diff --git a/spec/lib/gitlab/git/diff_collection_spec.rb b/spec/lib/gitlab/git/diff_collection_spec.rb index 3565e719ad3..a9a7bba2c05 100644 --- a/spec/lib/gitlab/git/diff_collection_spec.rb +++ b/spec/lib/gitlab/git/diff_collection_spec.rb @@ -341,7 +341,8 @@ describe Gitlab::Git::DiffCollection, seed_helper: true do end context 'when diff is quite large will collapse by default' do - let(:iterator) { [{ diff: 'a' * 20480 }] } + let(:iterator) { [{ diff: 'a' * (Gitlab::Git::Diff.collapse_limit + 1) }] } + let(:max_files) { 100 } context 'when no collapse is set' do let(:expanded) { true } diff --git a/spec/lib/gitlab/git/diff_spec.rb b/spec/lib/gitlab/git/diff_spec.rb index 8e24168ad71..6cff89b2f04 100644 --- a/spec/lib/gitlab/git/diff_spec.rb +++ b/spec/lib/gitlab/git/diff_spec.rb @@ -31,6 +31,36 @@ EOT [".gitmodules"]).patches.first end + describe 'size limit feature toggles' do + context 'when the feature gitlab_git_diff_size_limit_increase is enabled' do + before do + Feature.enable('gitlab_git_diff_size_limit_increase') + end + + it 'returns 200 KB for size_limit' do + expect(described_class.size_limit).to eq(200.kilobytes) + end + + it 'returns 100 KB for collapse_limit' do + expect(described_class.collapse_limit).to eq(100.kilobytes) + end + end + + context 'when the feature gitlab_git_diff_size_limit_increase is disabled' do + before do + Feature.disable('gitlab_git_diff_size_limit_increase') + end + + it 'returns 100 KB for size_limit' do + expect(described_class.size_limit).to eq(100.kilobytes) + end + + it 'returns 10 KB for collapse_limit' do + expect(described_class.collapse_limit).to eq(10.kilobytes) + end + end + end + describe '.new' do context 'using a Hash' do context 'with a small diff' do @@ -47,7 +77,7 @@ EOT context 'using a diff that is too large' do it 'prunes the diff' do - diff = described_class.new(diff: 'a' * 204800) + diff = described_class.new(diff: 'a' * (described_class.size_limit + 1)) expect(diff.diff).to be_empty expect(diff).to be_too_large @@ -85,8 +115,8 @@ EOT # The patch total size is 200, with lines between 21 and 54. # This is a quick-and-dirty way to test this. Ideally, a new patch is # added to the test repo with a size that falls between the real limits. - stub_const("#{described_class}::SIZE_LIMIT", 150) - stub_const("#{described_class}::COLLAPSE_LIMIT", 100) + allow(Gitlab::Git::Diff).to receive(:size_limit).and_return(150) + allow(Gitlab::Git::Diff).to receive(:collapse_limit).and_return(100) end it 'prunes the diff as a large diff instead of as a collapsed diff' do @@ -299,7 +329,7 @@ EOT describe '#collapsed?' do it 'returns true for a diff that is quite large' do - diff = described_class.new({ diff: 'a' * 20480 }, expanded: false) + diff = described_class.new({ diff: 'a' * (described_class.collapse_limit + 1) }, expanded: false) expect(diff).to be_collapsed end -- cgit v1.2.3