Commit 1d3c33b5 authored by Sean McGivern's avatar Sean McGivern

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.
parent f07aee72
---
title: Increase individual diff collapse limit to 100 KB, and render limit to 200 KB
merge_request:
author:
...@@ -27,6 +27,18 @@ class Feature ...@@ -27,6 +27,18 @@ class Feature
all.map(&:name).include?(feature.name) all.map(&:name).include?(feature.name)
end end
def enabled?(key)
get(key).enabled?
end
def enable(key)
get(key).enable
end
def disable(key)
get(key).disable
end
private private
def flipper def flipper
......
...@@ -20,13 +20,25 @@ module Gitlab ...@@ -20,13 +20,25 @@ module Gitlab
# We need this accessor because of `to_hash` and `init_from_hash` # We need this accessor because of `to_hash` and `init_from_hash`
attr_accessor :too_large attr_accessor :too_large
# The maximum size of a diff to display. class << self
SIZE_LIMIT = 100.kilobytes # 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. # The maximum size before a diff is collapsed.
COLLAPSE_LIMIT = 10.kilobytes 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) def between(repo, head, base, options = {}, *paths)
straight = options.delete(:straight) || false straight = options.delete(:straight) || false
...@@ -231,7 +243,7 @@ module Gitlab ...@@ -231,7 +243,7 @@ module Gitlab
def too_large? def too_large?
if @too_large.nil? if @too_large.nil?
@too_large = @diff.bytesize >= SIZE_LIMIT @too_large = @diff.bytesize >= self.class.size_limit
else else
@too_large @too_large
end end
...@@ -246,7 +258,7 @@ module Gitlab ...@@ -246,7 +258,7 @@ module Gitlab
def collapsed? def collapsed?
return @collapsed if defined?(@collapsed) return @collapsed if defined?(@collapsed)
@collapsed = !expanded && @diff.bytesize >= COLLAPSE_LIMIT @collapsed = !expanded && @diff.bytesize >= self.class.collapse_limit
end end
def collapse! def collapse!
...@@ -318,14 +330,14 @@ module Gitlab ...@@ -318,14 +330,14 @@ module Gitlab
hunk.each_line do |line| hunk.each_line do |line|
size += line.content.bytesize size += line.content.bytesize
if size >= SIZE_LIMIT if size >= self.class.size_limit
too_large! too_large!
return true return true
end end
end end
end end
if !expanded && size >= COLLAPSE_LIMIT if !expanded && size >= self.class.collapse_limit
collapse! collapse!
return true return true
end end
......
...@@ -5,6 +5,11 @@ feature 'Expand and collapse diffs', js: true, feature: true do ...@@ -5,6 +5,11 @@ feature 'Expand and collapse diffs', js: true, feature: true do
let(:project) { create(:project, :repository) } let(:project) { create(:project, :repository) }
before do 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 login_as :admin
# Ensure that undiffable.md is in .gitattributes # Ensure that undiffable.md is in .gitattributes
...@@ -62,18 +67,6 @@ feature 'Expand and collapse diffs', js: true, feature: true do ...@@ -62,18 +67,6 @@ feature 'Expand and collapse diffs', js: true, feature: true do
expect(small_diff).not_to have_selector('.nothing-here-block') expect(small_diff).not_to have_selector('.nothing-here-block')
end 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 it 'shows non-renderable diffs as such immediately, regardless of their size' do
expect(undiffable).not_to have_selector('.code') expect(undiffable).not_to have_selector('.code')
expect(undiffable).to have_selector('.nothing-here-block') expect(undiffable).to have_selector('.nothing-here-block')
......
...@@ -100,7 +100,7 @@ feature 'Merge request conflict resolution', js: true, feature: true do ...@@ -100,7 +100,7 @@ feature 'Merge request conflict resolution', js: true, feature: true do
context 'in Parallel view mode' do context 'in Parallel view mode' do
before do before do
click_link('conflicts', href: /\/conflicts\Z/) click_link('conflicts', href: /\/conflicts\Z/)
click_button 'Side-by-side' click_button 'Side-by-side'
end end
......
...@@ -24,6 +24,7 @@ describe "Compare", js: true do ...@@ -24,6 +24,7 @@ describe "Compare", js: true do
expect(find(".js-compare-to-dropdown .dropdown-toggle-text")).to have_content("binary-encoding") expect(find(".js-compare-to-dropdown .dropdown-toggle-text")).to have_content("binary-encoding")
click_button "Compare" click_button "Compare"
expect(page).to have_content "Commits" expect(page).to have_content "Commits"
end end
......
...@@ -341,7 +341,8 @@ describe Gitlab::Git::DiffCollection, seed_helper: true do ...@@ -341,7 +341,8 @@ describe Gitlab::Git::DiffCollection, seed_helper: true do
end end
context 'when diff is quite large will collapse by default' do 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 context 'when no collapse is set' do
let(:expanded) { true } let(:expanded) { true }
......
...@@ -31,6 +31,36 @@ EOT ...@@ -31,6 +31,36 @@ EOT
[".gitmodules"]).patches.first [".gitmodules"]).patches.first
end 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 describe '.new' do
context 'using a Hash' do context 'using a Hash' do
context 'with a small diff' do context 'with a small diff' do
...@@ -47,7 +77,7 @@ EOT ...@@ -47,7 +77,7 @@ EOT
context 'using a diff that is too large' do context 'using a diff that is too large' do
it 'prunes the diff' 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.diff).to be_empty
expect(diff).to be_too_large expect(diff).to be_too_large
...@@ -85,8 +115,8 @@ EOT ...@@ -85,8 +115,8 @@ EOT
# The patch total size is 200, with lines between 21 and 54. # 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 # 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. # added to the test repo with a size that falls between the real limits.
stub_const("#{described_class}::SIZE_LIMIT", 150) allow(Gitlab::Git::Diff).to receive(:size_limit).and_return(150)
stub_const("#{described_class}::COLLAPSE_LIMIT", 100) allow(Gitlab::Git::Diff).to receive(:collapse_limit).and_return(100)
end end
it 'prunes the diff as a large diff instead of as a collapsed diff' do it 'prunes the diff as a large diff instead of as a collapsed diff' do
...@@ -299,7 +329,7 @@ EOT ...@@ -299,7 +329,7 @@ EOT
describe '#collapsed?' do describe '#collapsed?' do
it 'returns true for a diff that is quite large' 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 expect(diff).to be_collapsed
end end
......
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