summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDouwe Maan <douwe@gitlab.com>2018-10-02 13:53:32 +0000
committerDouwe Maan <douwe@gitlab.com>2018-10-02 13:53:32 +0000
commit57dc233325727a0baf28492bf59ae4b67562b84d (patch)
tree82432be4497b31c018a95a667b56f85842ba2960
parent8ed357342ce6c99deea56f58c2587d679696f7f6 (diff)
parent4fbca2a346dc4c2c2c57e6a5bc3d13a8c3eeb23e (diff)
downloadgitlab-ce-57dc233325727a0baf28492bf59ae4b67562b84d.tar.gz
Merge branch 'osw-configurable-single-diff-file-limit' into 'master'
Make single diff patch limit configurable Closes #48027 See merge request gitlab-org/gitlab-ce!21886
-rw-r--r--app/helpers/application_settings_helper.rb3
-rw-r--r--app/models/application_setting.rb9
-rw-r--r--app/views/admin/application_settings/_diff_limits.html.haml16
-rw-r--r--app/views/admin/application_settings/show.html.haml11
-rw-r--r--changelogs/unreleased/osw-configurable-single-diff-file-limit.yml5
-rw-r--r--db/migrate/20180924141949_add_diff_max_patch_bytes_to_application_settings.rb25
-rw-r--r--db/schema.rb3
-rw-r--r--doc/administration/index.md1
-rw-r--r--doc/development/diffs.md27
-rw-r--r--doc/user/admin_area/diff_limits.md21
-rw-r--r--lib/gitlab/git/diff.rb62
-rw-r--r--lib/gitlab/git/diff_collection.rb2
-rw-r--r--locale/gitlab.pot6
-rw-r--r--spec/models/application_setting_spec.rb15
-rw-r--r--spec/requests/api/settings_spec.rb4
15 files changed, 158 insertions, 52 deletions
diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb
index c9a5431d18e..15cbfeea609 100644
--- a/app/helpers/application_settings_helper.rb
+++ b/app/helpers/application_settings_helper.rb
@@ -254,7 +254,8 @@ module ApplicationSettingsHelper
:user_default_internal_regex,
:user_oauth_applications,
:version_check_enabled,
- :web_ide_clientside_preview_enabled
+ :web_ide_clientside_preview_enabled,
+ :diff_max_patch_bytes
]
end
diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb
index 5f835a8da75..65a2f760f93 100644
--- a/app/models/application_setting.rb
+++ b/app/models/application_setting.rb
@@ -182,6 +182,12 @@ class ApplicationSetting < ActiveRecord::Base
numericality: { less_than_or_equal_to: :gitaly_timeout_default },
if: :gitaly_timeout_default
+ validates :diff_max_patch_bytes,
+ presence: true,
+ numericality: { only_integer: true,
+ greater_than_or_equal_to: Gitlab::Git::Diff::DEFAULT_MAX_PATCH_BYTES,
+ less_than_or_equal_to: Gitlab::Git::Diff::MAX_PATCH_BYTES_UPPER_BOUND }
+
validates :user_default_internal_regex, js_regex: true, allow_nil: true
SUPPORTED_KEY_TYPES.each do |type|
@@ -293,7 +299,8 @@ class ApplicationSetting < ActiveRecord::Base
user_default_external: false,
user_default_internal_regex: nil,
user_show_add_ssh_key_message: true,
- usage_stats_set_by_user_id: nil
+ usage_stats_set_by_user_id: nil,
+ diff_max_patch_bytes: Gitlab::Git::Diff::DEFAULT_MAX_PATCH_BYTES
}
end
diff --git a/app/views/admin/application_settings/_diff_limits.html.haml b/app/views/admin/application_settings/_diff_limits.html.haml
new file mode 100644
index 00000000000..408e569fe07
--- /dev/null
+++ b/app/views/admin/application_settings/_diff_limits.html.haml
@@ -0,0 +1,16 @@
+= form_for @application_setting, url: admin_application_settings_path(anchor: 'js-merge-request-settings'), html: { class: 'fieldset-form' } do |f|
+ = form_errors(@application_setting)
+
+ %fieldset
+ .form-group
+ = f.label :diff_max_patch_bytes, 'Maximum diff patch size (Bytes)', class: 'label-light'
+ = f.number_field :diff_max_patch_bytes, class: 'form-control'
+ %span.form-text.text-muted
+ Diff files surpassing this limit will be presented as 'too large'
+ and won't be expandable.
+
+ = link_to icon('question-circle'),
+ help_page_path('user/admin_area/diff_limits',
+ anchor: 'maximum-diff-patch-size')
+
+ = f.submit _('Save changes'), class: 'btn btn-success'
diff --git a/app/views/admin/application_settings/show.html.haml b/app/views/admin/application_settings/show.html.haml
index e2043183a97..761555c4189 100644
--- a/app/views/admin/application_settings/show.html.haml
+++ b/app/views/admin/application_settings/show.html.haml
@@ -24,6 +24,17 @@
.settings-content
= render 'account_and_limit'
+%section.settings.as-diff-limits.no-animate#js-merge-request-settings{ class: ('expanded' if expanded_by_default?) }
+ .settings-header
+ %h4
+ = _('Diff limits')
+ %button.btn.js-settings-toggle{ type: 'button' }
+ = expanded_by_default? ? _('Collapse') : _('Expand')
+ %p
+ = _('Diff content limits')
+ .settings-content
+ = render 'diff_limits'
+
%section.settings.as-signup.no-animate#js-signup-settings{ class: ('expanded' if expanded_by_default?) }
.settings-header
%h4
diff --git a/changelogs/unreleased/osw-configurable-single-diff-file-limit.yml b/changelogs/unreleased/osw-configurable-single-diff-file-limit.yml
new file mode 100644
index 00000000000..55a4b305885
--- /dev/null
+++ b/changelogs/unreleased/osw-configurable-single-diff-file-limit.yml
@@ -0,0 +1,5 @@
+---
+title: Make single diff patch limit configurable
+merge_request: 21886
+author:
+type: added
diff --git a/db/migrate/20180924141949_add_diff_max_patch_bytes_to_application_settings.rb b/db/migrate/20180924141949_add_diff_max_patch_bytes_to_application_settings.rb
new file mode 100644
index 00000000000..084dfc65ce5
--- /dev/null
+++ b/db/migrate/20180924141949_add_diff_max_patch_bytes_to_application_settings.rb
@@ -0,0 +1,25 @@
+# frozen_string_literal: true
+
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddDiffMaxPatchBytesToApplicationSettings < ActiveRecord::Migration
+ include Gitlab::Database::MigrationHelpers
+
+ # Set this constant to true if this migration requires downtime.
+ DOWNTIME = false
+
+ disable_ddl_transaction!
+
+ def up
+ add_column_with_default(:application_settings,
+ :diff_max_patch_bytes,
+ :integer,
+ default: 100.kilobytes,
+ allow_null: false)
+ end
+
+ def down
+ remove_column(:application_settings, :diff_max_patch_bytes)
+ end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 13c6d65255e..b3d4badaf82 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
#
# It's strongly recommended that you check this file into your version control system.
-ActiveRecord::Schema.define(version: 20180917172041) do
+ActiveRecord::Schema.define(version: 20180924141949) do
# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"
@@ -171,6 +171,7 @@ ActiveRecord::Schema.define(version: 20180917172041) do
t.boolean "user_show_add_ssh_key_message", default: true, null: false
t.integer "usage_stats_set_by_user_id"
t.integer "receive_max_input_size"
+ t.integer "diff_max_patch_bytes", default: 102400, null: false
end
create_table "audit_events", force: :cascade do |t|
diff --git a/doc/administration/index.md b/doc/administration/index.md
index 702d8e554a8..d713247983b 100644
--- a/doc/administration/index.md
+++ b/doc/administration/index.md
@@ -47,6 +47,7 @@ Learn how to install, configure, update, and maintain your GitLab instance.
- [Enforcing Terms of Service](../user/admin_area/settings/terms.md)
- [Third party offers](../user/admin_area/settings/third_party_offers.md)
- [Compliance](compliance.md): A collection of features from across the application that you may configure to help ensure that your GitLab instance and DevOps workflow meet compliance standards.
+- [Diff limits](../user/admin_area/diff_limits.md): Configure the diff rendering size limits of branch comparison pages.
#### Customizing GitLab's appearance
diff --git a/doc/development/diffs.md b/doc/development/diffs.md
index 5e8e8cc7541..4adae5dabe2 100644
--- a/doc/development/diffs.md
+++ b/doc/development/diffs.md
@@ -2,13 +2,10 @@
Currently we rely on different sources to present diffs, these include:
-- Rugged gem
- Gitaly service
- Database (through `merge_request_diff_files`)
- Redis (cached highlighted diffs)
-We're constantly moving Rugged calls to Gitaly and the progress can be followed through [Gitaly repo](https://gitlab.com/gitlab-org/gitaly).
-
## Architecture overview
### Merge request diffs
@@ -19,8 +16,9 @@ we fetch the comparison information using `Gitlab::Git::Compare`, which fetches
The diffs fetching process _limits_ single file diff sizes and the overall size of the whole diff through a series of constant values. Raw diff files are
then persisted on `merge_request_diff_files` table.
-Even though diffs higher than 10kb are collapsed (`Gitlab::Git::Diff::COLLAPSE_LIMIT`), we still keep them on Postgres. However, diff files over _safety limits_
-(see the [Diff limits section](#diff-limits)) are _not_ persisted.
+Even though diffs larger than 10% of the value of `ApplicationSettings#diff_max_patch_bytes` are collapsed,
+we still keep them on Postgres. However, diff files larger than defined _safety limits_
+(see the [Diff limits section](#diff-limits)) are _not_ persisted in the database.
In order to present diffs information on the Merge Request diffs page, we:
@@ -102,23 +100,20 @@ Gitaly will only return the safe amount of data to be persisted on `merge_reques
Limits that act onto each diff file of a collection. Files number, lines number and files size are considered.
-```ruby
-Gitlab::Git::Diff::COLLAPSE_LIMIT = 10.kilobytes
-```
+#### Expandable patches (collapsed)
-File diff will be collapsed (but be expandable) if it is larger than 10 kilobytes.
+Diff patches are collapsed when surpassing 10% of the value set in `ApplicationSettings#diff_max_patch_bytes`.
+That is, it's equivalent to 10kb if the maximum allowed value is 100kb.
+The diff will still be persisted and expandable if the patch size doesn't
+surpass `ApplicationSettings#diff_max_patch_bytes`.
*Note:* Although this nomenclature (Collapsing) is also used on Gitaly, this limit is only used on GitLab (hardcoded - not sent to Gitaly).
Gitaly will only return `Diff.Collapsed` (RPC) when surpassing collection limits.
-```ruby
-Gitlab::Git::Diff::SIZE_LIMIT = 100.kilobytes
-```
-
-File diff will not be rendered if it's larger than 100 kilobytes.
+#### Not expandable patches (too large)
-*Note:* This limit is currently hardcoded and applied on Gitaly and the RPC returns `Diff.TooLarge` when this limit is surpassed.
-Although we're still also applying it on GitLab, we should remove the redundancy from GitLab once we're confident with the Gitaly integration.
+The patch not be rendered if it's larger than `ApplicationSettings#diff_max_patch_bytes`.
+Users will see a `This source diff could not be displayed because it is too large` message.
```ruby
Commit::DIFF_SAFE_LINES = Gitlab::Git::DiffCollection::DEFAULT_LIMITS[:max_lines] = 5000
diff --git a/doc/user/admin_area/diff_limits.md b/doc/user/admin_area/diff_limits.md
new file mode 100644
index 00000000000..9205860ef1f
--- /dev/null
+++ b/doc/user/admin_area/diff_limits.md
@@ -0,0 +1,21 @@
+# Diff limits administration
+
+NOTE: **Note:**
+Merge requests and branch comparison views will be affected.
+
+CAUTION: **Caution:**
+These settings are currently under experimental state. They'll
+increase the resource consumption of your instance and should
+be edited mindfully.
+
+1. Access **Admin area > Settings > General**
+1. Expand **Diff limits**
+
+### Maximum diff patch size
+
+This is the content size each diff file (patch) is allowed to reach before
+it's collapsed, without the possibility of being expanded. A link redirecting
+to the blob view will be presented for the patches that surpass this limit.
+
+Patches surpassing 10% of this content size will be automatically collapsed,
+but expandable (a link to expand the diff will be presented).
diff --git a/lib/gitlab/git/diff.rb b/lib/gitlab/git/diff.rb
index f6b51dc3982..0d96211f4d4 100644
--- a/lib/gitlab/git/diff.rb
+++ b/lib/gitlab/git/diff.rb
@@ -19,13 +19,17 @@ module Gitlab
alias_method :expanded?, :expanded
- SERIALIZE_KEYS = %i(diff new_path old_path a_mode b_mode new_file renamed_file deleted_file too_large).freeze
+ # The default maximum content size to display a diff patch.
+ #
+ # If this value ever changes, make sure to create a migration to update
+ # current records, and default of `ApplicationSettings#diff_max_patch_bytes`.
+ DEFAULT_MAX_PATCH_BYTES = 100.kilobytes
- # The maximum size of a diff to display.
- SIZE_LIMIT = 100.kilobytes
+ # This is a limitation applied on the source (Gitaly), therefore we don't allow
+ # persisting limits over that.
+ MAX_PATCH_BYTES_UPPER_BOUND = 500.kilobytes
- # The maximum size before a diff is collapsed.
- COLLAPSE_LIMIT = 10.kilobytes
+ SERIALIZE_KEYS = %i(diff new_path old_path a_mode b_mode new_file renamed_file deleted_file too_large).freeze
class << self
def between(repo, head, base, options = {}, *paths)
@@ -105,6 +109,26 @@ module Gitlab
def binary_message(old_path, new_path)
"Binary files #{old_path} and #{new_path} differ\n"
end
+
+ # Returns the limit of bytes a single diff file can reach before it
+ # appears as 'collapsed' for end-users.
+ # By convention, it's 10% of the persisted `diff_max_patch_bytes`.
+ #
+ # Example: If we have 100k for the `diff_max_patch_bytes`, it will be 10k by
+ # default.
+ #
+ # Patches surpassing this limit should still be persisted in the database.
+ def patch_safe_limit_bytes
+ patch_hard_limit_bytes / 10
+ end
+
+ # Returns the limit for a single diff file (patch).
+ #
+ # Patches surpassing this limit shouldn't be persisted in the database
+ # and will be presented as 'too large' for end-users.
+ def patch_hard_limit_bytes
+ Gitlab::CurrentSettings.diff_max_patch_bytes
+ end
end
def initialize(raw_diff, expanded: true)
@@ -150,7 +174,7 @@ module Gitlab
def too_large?
if @too_large.nil?
- @too_large = @diff.bytesize >= SIZE_LIMIT
+ @too_large = @diff.bytesize >= self.class.patch_hard_limit_bytes
else
@too_large
end
@@ -168,7 +192,7 @@ module Gitlab
def collapsed?
return @collapsed if defined?(@collapsed)
- @collapsed = !expanded && @diff.bytesize >= COLLAPSE_LIMIT
+ @collapsed = !expanded && @diff.bytesize >= self.class.patch_safe_limit_bytes
end
def collapse!
@@ -219,30 +243,6 @@ module Gitlab
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)
- size = 0
-
- patch.each_hunk do |hunk|
- hunk.each_line do |line|
- size += line.content.bytesize
-
- if size >= SIZE_LIMIT
- too_large!
- return true # rubocop:disable Cop/AvoidReturnFromBlocks
- end
- end
- end
-
- if !expanded && size >= COLLAPSE_LIMIT
- collapse!
- return true
- end
-
- false
- end
end
end
end
diff --git a/lib/gitlab/git/diff_collection.rb b/lib/gitlab/git/diff_collection.rb
index 20dce8d0e06..47ebca7c4a2 100644
--- a/lib/gitlab/git/diff_collection.rb
+++ b/lib/gitlab/git/diff_collection.rb
@@ -19,7 +19,7 @@ module Gitlab
limits[:safe_max_files] = [limits[:max_files], DEFAULT_LIMITS[:max_files]].min
limits[:safe_max_lines] = [limits[:max_lines], DEFAULT_LIMITS[:max_lines]].min
limits[:safe_max_bytes] = limits[:safe_max_files] * 5.kilobytes # Average 5 KB per file
- limits[:max_patch_bytes] = Gitlab::Git::Diff::SIZE_LIMIT
+ limits[:max_patch_bytes] = Gitlab::Git::Diff.patch_hard_limit_bytes
OpenStruct.new(limits)
end
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index a9c5e45f9fa..01448223a7d 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -2292,6 +2292,12 @@ msgstr ""
msgid "Details"
msgstr ""
+msgid "Diff content limits"
+msgstr ""
+
+msgid "Diff limits"
+msgstr ""
+
msgid "Diffs|No file name available"
msgstr ""
diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb
index 9647c1b9f63..dcfc80daa57 100644
--- a/spec/models/application_setting_spec.rb
+++ b/spec/models/application_setting_spec.rb
@@ -592,4 +592,19 @@ describe ApplicationSetting do
it { is_expected.to eq(result) }
end
end
+
+ context 'diff limit settings' do
+ describe '#diff_max_patch_bytes' do
+ context 'validations' do
+ it { is_expected.to validate_presence_of(:diff_max_patch_bytes) }
+
+ it do
+ is_expected.to validate_numericality_of(:diff_max_patch_bytes)
+ .only_integer
+ .is_greater_than_or_equal_to(Gitlab::Git::Diff::DEFAULT_MAX_PATCH_BYTES)
+ .is_less_than_or_equal_to(Gitlab::Git::Diff::MAX_PATCH_BYTES_UPPER_BOUND)
+ end
+ end
+ end
+ end
end
diff --git a/spec/requests/api/settings_spec.rb b/spec/requests/api/settings_spec.rb
index 3e0f47b84a1..e379bd9785a 100644
--- a/spec/requests/api/settings_spec.rb
+++ b/spec/requests/api/settings_spec.rb
@@ -66,7 +66,8 @@ describe API::Settings, 'Settings' do
enforce_terms: true,
terms: 'Hello world!',
performance_bar_allowed_group_path: group.full_path,
- instance_statistics_visibility_private: true
+ instance_statistics_visibility_private: true,
+ diff_max_patch_bytes: 150_000
expect(response).to have_gitlab_http_status(200)
expect(json_response['default_projects_limit']).to eq(3)
@@ -92,6 +93,7 @@ describe API::Settings, 'Settings' do
expect(json_response['terms']).to eq('Hello world!')
expect(json_response['performance_bar_allowed_group_id']).to eq(group.id)
expect(json_response['instance_statistics_visibility_private']).to be(true)
+ expect(json_response['diff_max_patch_bytes']).to eq(150_000)
end
end