Commit aed0387f authored by Douwe Maan's avatar Douwe Maan

Consistent diff and blob size limit names

parent 1ac12698
Pipeline #8622327 failed with stages
in 54 minutes and 37 seconds
......@@ -18,7 +18,7 @@ module RendersBlob
}
end
def override_max_blob_size(blob)
blob.override_max_size! if params[:override_max_size] == 'true'
def conditionally_expand_blob(blob)
blob.expand! if params[:expanded] == 'true'
end
end
......@@ -27,7 +27,7 @@ class Projects::ArtifactsController < Projects::ApplicationController
def file
blob = @entry.blob
override_max_blob_size(blob)
conditionally_expand_blob(blob)
respond_to do |format|
format.html do
......
......@@ -35,7 +35,7 @@ class Projects::BlobController < Projects::ApplicationController
end
def show
override_max_blob_size(@blob)
conditionally_expand_blob(@blob)
respond_to do |format|
format.html do
......
......@@ -56,7 +56,7 @@ class Projects::SnippetsController < Projects::ApplicationController
def show
blob = @snippet.blob
override_max_blob_size(blob)
conditionally_expand_blob(blob)
respond_to do |format|
format.html do
......
......@@ -58,7 +58,7 @@ class SnippetsController < ApplicationController
def show
blob = @snippet.blob
override_max_blob_size(blob)
conditionally_expand_blob(blob)
@note = Note.new(noteable: @snippet)
@noteable = @snippet
......
......@@ -240,14 +240,10 @@ module BlobHelper
def blob_render_error_reason(viewer)
case viewer.render_error
when :collapsed
"it is larger than #{number_to_human_size(viewer.collapse_limit)}"
when :too_large
max_size =
if viewer.can_override_max_size?
viewer.overridable_max_size
else
viewer.max_size
end
"it is larger than #{number_to_human_size(max_size)}"
"it is larger than #{number_to_human_size(viewer.size_limit)}"
when :server_side_but_stored_externally
case viewer.blob.external_storage
when :lfs
......@@ -264,8 +260,8 @@ module BlobHelper
error = viewer.render_error
options = []
if error == :too_large && viewer.can_override_max_size?
options << link_to('load it anyway', url_for(params.merge(viewer: viewer.type, override_max_size: true, format: nil)))
if error == :collapsed
options << link_to('load it anyway', url_for(params.merge(viewer: viewer.type, expanded: true, format: nil)))
end
# If the error is `:server_side_but_stored_externally`, the simple viewer will show the same error,
......
......@@ -8,8 +8,8 @@ module DiffHelper
[marked_old_line, marked_new_line]
end
def expand_all_diffs?
params[:expand_all_diffs].present?
def diffs_expanded?
params[:expanded].present?
end
def diff_view
......@@ -22,10 +22,10 @@ module DiffHelper
end
def diff_options
options = { ignore_whitespace_change: hide_whitespace?, no_collapse: expand_all_diffs? }
options = { ignore_whitespace_change: hide_whitespace?, expanded: diffs_expanded? }
if action_name == 'diff_for_path'
options[:no_collapse] = true
options[:expanded] = true
options[:paths] = params.values_at(:old_path, :new_path)
end
......
......@@ -102,10 +102,6 @@ class Blob < SimpleDelegator
raw_size == 0
end
def too_large?
size && truncated?
end
def external_storage_error?
if external_storage == :lfs
!project&.lfs_enabled?
......@@ -160,7 +156,7 @@ class Blob < SimpleDelegator
end
def readable_text?
text? && !stored_externally? && !too_large?
text? && !stored_externally? && !truncated?
end
def simple_viewer
......@@ -187,9 +183,9 @@ class Blob < SimpleDelegator
rendered_as_text? && rich_viewer
end
def override_max_size!
simple_viewer&.override_max_size = true
rich_viewer&.override_max_size = true
def expand!
simple_viewer&.expanded = true
rich_viewer&.expanded = true
end
private
......
......@@ -7,8 +7,8 @@ module BlobViewer
included do
self.loading_partial_name = 'loading_auxiliary'
self.type = :auxiliary
self.overridable_max_size = 100.kilobytes
self.max_size = 100.kilobytes
self.collapse_limit = 100.kilobytes
self.size_limit = 100.kilobytes
end
def visible_to?(current_user)
......
......@@ -2,14 +2,14 @@ module BlobViewer
class Base
PARTIAL_PATH_PREFIX = 'projects/blob/viewers'.freeze
class_attribute :partial_name, :loading_partial_name, :type, :extensions, :file_types, :load_async, :binary, :switcher_icon, :switcher_title, :overridable_max_size, :max_size
class_attribute :partial_name, :loading_partial_name, :type, :extensions, :file_types, :load_async, :binary, :switcher_icon, :switcher_title, :collapse_limit, :size_limit
self.loading_partial_name = 'loading'
delegate :partial_path, :loading_partial_path, :rich?, :simple?, :text?, :binary?, to: :class
attr_reader :blob
attr_accessor :override_max_size
attr_accessor :expanded
delegate :project, to: :blob
......@@ -61,24 +61,16 @@ module BlobViewer
self.class.load_async? && render_error.nil?
end
def exceeds_overridable_max_size?
overridable_max_size && blob.raw_size > overridable_max_size
end
def exceeds_max_size?
max_size && blob.raw_size > max_size
end
def collapsed?
return @collapsed if defined?(@collapsed)
def can_override_max_size?
exceeds_overridable_max_size? && !exceeds_max_size?
@collapsed = !expanded && collapse_limit && blob.raw_size > collapse_limit
end
def too_large?
if override_max_size
exceeds_max_size?
else
exceeds_overridable_max_size?
end
return @too_large if defined?(@too_large)
@too_large = size_limit && blob.raw_size > size_limit
end
# This method is used on the server side to check whether we can attempt to
......@@ -95,6 +87,8 @@ module BlobViewer
def render_error
if too_large?
:too_large
elsif collapsed?
:collapsed
end
end
......
......@@ -4,8 +4,8 @@ module BlobViewer
included do
self.load_async = false
self.overridable_max_size = 10.megabytes
self.max_size = 50.megabytes
self.collapse_limit = 10.megabytes
self.size_limit = 50.megabytes
end
end
end
......@@ -4,8 +4,8 @@ module BlobViewer
included do
self.load_async = true
self.overridable_max_size = 2.megabytes
self.max_size = 5.megabytes
self.collapse_limit = 2.megabytes
self.size_limit = 5.megabytes
end
def prepare!
......
......@@ -5,7 +5,7 @@ module BlobViewer
self.partial_name = 'text'
self.binary = false
self.overridable_max_size = 1.megabyte
self.max_size = 10.megabytes
self.collapse_limit = 1.megabyte
self.size_limit = 10.megabytes
end
end
......@@ -220,10 +220,10 @@ class MergeRequest < ActiveRecord::Base
def diffs(diff_options = {})
if compare
# When saving MR diffs, `no_collapse` is implicitly added (because we need
# When saving MR diffs, `expanded` is implicitly added (because we need
# to save the entire contents to the DB), so add that here for
# consistency.
compare.diffs(diff_options.merge(no_collapse: true))
compare.diffs(diff_options.merge(expanded: true))
else
merge_request_diff.diffs(diff_options)
end
......
......@@ -3,7 +3,7 @@
.diff-content
- if diff_file.too_large?
.nothing-here-block This diff could not be displayed because it is too large.
- elsif blob.too_large?
- elsif blob.truncated?
.nothing-here-block The file could not be displayed because it is too large.
- elsif blob.readable_text?
- if !diff_file.repository.diffable?(blob)
......
......@@ -5,8 +5,8 @@
.content-block.oneline-block.files-changed
.inline-parallel-buttons
- if !expand_all_diffs? && diff_files.any? { |diff_file| diff_file.collapsed? }
= link_to 'Expand all', url_for(params.merge(expand_all_diffs: 1, format: nil)), class: 'btn btn-default'
- if !diffs_expanded? && diff_files.any? { |diff_file| diff_file.collapsed? }
= link_to 'Expand all', url_for(params.merge(expanded: 1, format: nil)), class: 'btn btn-default'
- if show_whitespace_toggle
- if current_controller?(:commit)
= commit_diff_whitespace_link(diffs.project, @commit, class: 'hidden-xs')
......
......@@ -176,7 +176,7 @@ module API
}
if params[:path]
commit.raw_diffs(all_diffs: true).each do |diff|
commit.raw_diffs(no_limits: true).each do |diff|
next unless diff.new_path == params[:path]
lines = Gitlab::Diff::Parser.new.parse(diff.diff.each_line)
......
......@@ -331,7 +331,7 @@ module API
class MergeRequestChanges < MergeRequest
expose :diffs, as: :changes, using: Entities::RepoDiff do |compare, _|
compare.raw_diffs(all_diffs: true).to_a
compare.raw_diffs(no_limits: true).to_a
end
end
......@@ -344,7 +344,7 @@ module API
expose :commits, using: Entities::RepoCommit
expose :diffs, using: Entities::RepoDiff do |compare, _|
compare.raw_diffs(all_diffs: true).to_a
compare.raw_diffs(no_limits: true).to_a
end
end
......@@ -548,7 +548,7 @@ module API
end
expose :diffs, using: Entities::RepoDiff do |compare, options|
compare.diffs(all_diffs: true).to_a
compare.diffs(no_limits: true).to_a
end
expose :compare_timeout do |compare, options|
......
......@@ -167,7 +167,7 @@ module API
}
if params[:path]
commit.raw_diffs(all_diffs: true).each do |diff|
commit.raw_diffs(no_limits: true).each do |diff|
next unless diff.new_path == params[:path]
lines = Gitlab::Diff::Parser.new.parse(diff.diff.each_line)
......
......@@ -226,7 +226,7 @@ module API
class MergeRequestChanges < MergeRequest
expose :diffs, as: :changes, using: ::API::Entities::RepoDiff do |compare, _|
compare.raw_diffs(all_diffs: true).to_a
compare.raw_diffs(no_limits: true).to_a
end
end
......
......@@ -7,7 +7,7 @@ module Gitlab
delegate :count, :size, :real_size, to: :diff_files
def self.default_options
::Commit.max_diff_options.merge(ignore_whitespace_change: false, no_collapse: false)
::Commit.max_diff_options.merge(ignore_whitespace_change: false, expanded: false)
end
def initialize(diffable, project:, diff_options: nil, diff_refs: nil, fallback_diff_refs: nil)
......
......@@ -42,7 +42,7 @@ module Gitlab
return unless compare
# This diff is more moderated in number of files and lines
@diffs ||= compare.diffs(max_files: 30, max_lines: 5000, no_collapse: true).diff_files
@diffs ||= compare.diffs(max_files: 30, max_lines: 5000, expanded: true).diff_files
end
def diffs_count
......
......@@ -88,6 +88,7 @@ module Gitlab
new(
id: blob_entry[:oid],
name: blob_entry[:name],
size: 0,
data: '',
path: path,
commit_id: sha
......
......@@ -15,13 +15,13 @@ module Gitlab
alias_method :deleted_file?, :deleted_file
alias_method :renamed_file?, :renamed_file
attr_accessor :too_large
attr_accessor :expanded
# The maximum size of a diff to display.
DIFF_SIZE_LIMIT = 102400 # 100 KB
SIZE_LIMIT = 100.kilobytes
# The maximum size before a diff is collapsed.
DIFF_COLLAPSE_LIMIT = 10240 # 10 KB
COLLAPSE_LIMIT = 10.kilobytes
class << self
def between(repo, head, base, options = {}, *paths)
......@@ -152,7 +152,7 @@ module Gitlab
:include_untracked_content, :skip_binary_check,
:include_typechange, :include_typechange_trees,
:ignore_filemode, :recurse_ignored_dirs, :paths,
:max_files, :max_lines, :all_diffs, :no_collapse]
:max_files, :max_lines, :no_limits, :expanded]
if default_options
actual_defaults = default_options.dup
......@@ -177,16 +177,18 @@ module Gitlab
end
end
def initialize(raw_diff, collapse: false)
def initialize(raw_diff, expanded: true)
@expanded = expanded
case raw_diff
when Hash
init_from_hash(raw_diff)
prune_diff_if_eligible(collapse)
prune_diff_if_eligible
when Rugged::Patch, Rugged::Diff::Delta
init_from_rugged(raw_diff, collapse: collapse)
init_from_rugged(raw_diff)
when Gitaly::CommitDiffResponse
init_from_gitaly(raw_diff)
prune_diff_if_eligible(collapse)
prune_diff_if_eligible
when Gitaly::CommitDelta
init_from_gitaly(raw_diff)
when nil
......@@ -225,18 +227,12 @@ module Gitlab
end
def too_large?
if @too_large.nil?
@too_large = @diff.bytesize >= DIFF_SIZE_LIMIT
else
@too_large
end
end
return @too_large if defined?(@too_large)
def collapsible?
@diff.bytesize >= DIFF_COLLAPSE_LIMIT
@too_large = @diff.bytesize >= SIZE_LIMIT
end
def prune_large_diff!
def too_large!
@diff = ''
@line_count = 0
@too_large = true
......@@ -244,10 +240,11 @@ module Gitlab
def collapsed?
return @collapsed if defined?(@collapsed)
false
@collapsed = !expanded && @diff.bytesize >= COLLAPSE_LIMIT
end
def prune_collapsed_diff!
def collapse!
@diff = ''
@line_count = 0
@collapsed = true
......@@ -255,9 +252,9 @@ module Gitlab
private
def init_from_rugged(rugged, collapse: false)
def init_from_rugged(rugged)
if rugged.is_a?(Rugged::Patch)
init_from_rugged_patch(rugged, collapse: collapse)
init_from_rugged_patch(rugged)
d = rugged.delta
else
d = rugged
......@@ -272,10 +269,10 @@ module Gitlab
@deleted_file = d.deleted?
end
def init_from_rugged_patch(patch, collapse: false)
def init_from_rugged_patch(patch)
# Don't bother initializing diffs that are too large. If a diff is
# binary we're not going to display anything so we skip the size check.
return if !patch.delta.binary? && prune_large_patch(patch, collapse)
return if !patch.delta.binary? && prune_large_patch(patch)
@diff = encode!(strip_diff_headers(patch.to_s))
end
......@@ -299,29 +296,32 @@ module Gitlab
@deleted_file = msg.to_id == BLANK_SHA
end
def prune_diff_if_eligible(collapse = false)
prune_large_diff! if too_large?
prune_collapsed_diff! if collapse && collapsible?
def prune_diff_if_eligible
if too_large?
too_large!
elsif collapsed?
collapse!
end
end
# If the patch surpasses any of the diff limits it calls the appropiate
# prune method and returns true. Otherwise returns false.
def prune_large_patch(patch, collapse)
def prune_large_patch(patch)
size = 0
patch.each_hunk do |hunk|
hunk.each_line do |line|
size += line.content.bytesize
if size >= DIFF_SIZE_LIMIT
prune_large_diff!
if size >= SIZE_LIMIT
too_large!
return true
end
end
end
if collapse && size >= DIFF_COLLAPSE_LIMIT
prune_collapsed_diff!
if !expanded && size >= COLLAPSE_LIMIT
collapse!
return true
end
......
......@@ -9,12 +9,12 @@ module Gitlab
@iterator = iterator
@max_files = options.fetch(:max_files, DEFAULT_LIMITS[:max_files])
@max_lines = options.fetch(:max_lines, DEFAULT_LIMITS[:max_lines])
@max_bytes = @max_files * 5120 # Average 5 KB per file
@max_bytes = @max_files * 5.kilobytes # Average 5 KB per file
@safe_max_files = [@max_files, DEFAULT_LIMITS[:max_files]].min
@safe_max_lines = [@max_lines, DEFAULT_LIMITS[:max_lines]].min
@safe_max_bytes = @safe_max_files * 5120 # Average 5 KB per file
@all_diffs = !!options.fetch(:all_diffs, false)
@no_collapse = !!options.fetch(:no_collapse, true)
@safe_max_bytes = @safe_max_files * 5.kilobytes # Average 5 KB per file
@no_limits = !!options.fetch(:no_limits, false)
@expanded = !!options.fetch(:expanded, true)
@line_count = 0
@byte_count = 0
......@@ -88,23 +88,23 @@ module Gitlab
@iterator.each do |raw|
@empty = false
if !@all_diffs && i >= @max_files
if !@no_limits && i >= @max_files
@overflow = true
break
end
collapse = !@all_diffs && !@no_collapse
expanded = @no_limits || @expanded
diff = Gitlab::Git::Diff.new(raw, collapse: collapse)
diff = Gitlab::Git::Diff.new(raw, expanded: expanded)
if collapse && over_safe_limits?(i)
diff.prune_collapsed_diff!
if !expanded && over_safe_limits?(i)
diff.collapse!
end
@line_count += diff.line_count
@byte_count += diff.diff.bytesize
if !@all_diffs && (@line_count >= @max_lines || @byte_count >= @max_bytes)
if !@no_limits && (@line_count >= @max_lines || @byte_count >= @max_bytes)
# This last Diff instance pushes us over the lines limit. We stop and
# discard it.
@overflow = true
......
......@@ -118,8 +118,8 @@ describe BlobHelper do
Class.new(BlobViewer::Base) do
include BlobViewer::ServerSide
self.overridable_max_size = 1.megabyte
self.max_size = 5.megabytes
self.collapse_limit = 1.megabyte
self.size_limit = 5.megabytes
self.type = :rich
end
end
......@@ -129,7 +129,7 @@ describe BlobHelper do
describe '#blob_render_error_reason' do
context 'for error :too_large' do
context 'when the blob size is larger than the absolute max size' do
context 'when the blob size is larger than the absolute size limit' do
let(:blob) { fake_blob(size: 10.megabytes) }
it 'returns an error message' do
......@@ -137,7 +137,7 @@ describe BlobHelper do
end
end
context 'when the blob size is larger than the max size' do
context 'when the blob size is larger than the size limit' do
let(:blob) { fake_blob(size: 2.megabytes) }
it 'returns an error message' do
......@@ -169,7 +169,7 @@ describe BlobHelper do
end
context 'for error :too_large' do
context 'when the max size can be overridden' do
context 'when the size limit can be overridden' do
let(:blob) { fake_blob(size: 2.megabytes) }
it 'includes a "load it anyway" link' do
......@@ -177,7 +177,7 @@ describe BlobHelper do
end
end
context 'when the max size cannot be overridden' do
context 'when the size limit cannot be overridden' do
let(:blob) { fake_blob(size: 10.megabytes) }
it 'does not include a "load it anyway" link' do
......
......@@ -33,17 +33,17 @@ describe DiffHelper do
describe 'diff_options' do
it 'returns no collapse false' do
expect(diff_options).to include(no_collapse: false)
expect(diff_options).to include(expanded: false)
end
it 'returns no collapse true if expand_all_diffs' do
allow(controller).to receive(:params) { { expand_all_diffs: true } }
expect(diff_options).to include(no_collapse: true)
it 'returns no collapse true if expanded' do
allow(controller).to receive(:params) { { expanded: true } }
expect(diff_options).to include(expanded: true)
end
it 'returns no collapse true if action name diff_for_path' do
allow(controller).to receive(:action_name) { 'diff_for_path' }
expect(diff_options).to include(no_collapse: true)
expect(diff_options).to include(expanded: true)
end
it 'returns paths if action name diff_for_path and param old path' do
......
......@@ -6,8 +6,8 @@ describe Gitlab::Git::DiffCollection, seed_helper: true do
iterator,
max_files: max_files,
max_lines: max_lines,
all_diffs: all_diffs,
no_collapse: no_collapse
no_limits: no_limits,
expanded: expanded
)
end
let(:iterator) { MutatingConstantIterator.new(file_count, fake_diff(line_length, line_count)) }
......@@ -16,8 +16,8 @@ describe Gitlab::Git::DiffCollection, seed_helper: true do
let(:line_count) { 1 }
let(:max_files) { 10 }
let(:max_lines) { 100 }
let(:all_diffs) { false }
let(:no_collapse) { true }
let(:no_limits) { false }
let(:expanded) { true }
describe '#to_a' do
subject { super().to_a }
......@@ -75,7 +75,7 @@ describe Gitlab::Git::DiffCollection, seed_helper: true do
end
context 'when limiting is disabled' do
let(:all_diffs) { true }
let(:no_limits) { true }
describe '#overflow?' do
subject { super().overflow? }
......@@ -123,7 +123,7 @@ describe Gitlab::Git::DiffCollection, seed_helper: true do
it { expect(subject.size).to eq(0) }
context 'when limiting is disabled' do
let(:all_diffs) { true }
let(:no_limits) { true }
describe '#overflow?' do
subject { super().overflow? }
......@@ -167,7 +167,7 @@ describe Gitlab::Git::DiffCollection, seed_helper: true do
it { expect(subject.size).to eq(10) }
context 'when limiting is disabled' do
let(:all_diffs) { true }
let(:no_limits) { true }
describe '#overflow?' do
subject { super().overflow? }
......@@ -207,7 +207,7 @@ describe Gitlab::Git::DiffCollection, seed_helper: true do
it { expect(subject.size).to eq(3) }
context 'when limiting is disabled' do
let(:all_diffs) { true }
let(:no_limits) { true }
describe '#overflow?' do
subject { super().overflow? }
......@@ -273,7 +273,7 @@ describe Gitlab::Git::DiffCollection, seed_helper: true do
it { expect(subject.size).to eq(9) }
context 'when limiting is disabled' do
let(:all_diffs) { true }
let(:no_limits) { true }
describe '#overflow?' do
subject { super().overflow? }
......@@ -344,7 +344,7 @@ describe Gitlab::Git::DiffCollection, seed_helper: true do
let(:iterator) { [{ diff: 'a' * 20480 }] }
context 'when no collapse is set' do
let(:no_collapse) { true }
let(:expanded) { true }
it 'yields Diff instances even when they are quite big' do
expect { |b| subject.each(&b) }.
......@@ -363,7 +363,7 @@ describe Gitlab::Git::DiffCollection, seed_helper: true do
end
context 'when no collapse is unset' do
let(:no_collapse) { false }
let(:expanded) { false }
it 'yields Diff instances even when they are quite big' do
expect { |b| subject.each(&b) }.
......@@ -450,7 +450,7 @@ describe Gitlab::Git::DiffCollection, seed_helper: true do
end
context 'when limiting is disabled' do
let(:all_diffs) { true }
let(:no_limits) { true }
it 'yields Diff instances even when they are quite big' do
expect { |b| subject.each(&b) }.
......
......@@ -85,8 +85,8 @@ EOT
# The patch total size is 200, with lines between 21 and 54.