From c1880ec4dd4e66144e8e00552f60a8253ef48d73 Mon Sep 17 00:00:00 2001 From: Marc Shaw Date: Tue, 16 Mar 2021 20:18:21 +1300 Subject: [PATCH 1/2] Make max diff files and max diff lines configurable Issue: gitlab.com/gitlab-org/gitlab/-/issues/31063 Mr: gitlab.com/gitlab-org/gitlab/-/merge_requests/56722 Changelog: added --- app/helpers/application_settings_helper.rb | 2 + app/models/application_setting.rb | 12 +++ .../application_setting_implementation.rb | 2 + app/models/commit.rb | 28 ++++-- .../_diff_limits.html.haml | 25 +++++- .../development/configurable_diff_limits.yml | 8 ++ ..._diff_max_lines_to_application_settings.rb | 11 +++ ..._diff_max_files_to_application_settings.rb | 11 +++ db/schema_migrations/20210527133919 | 1 + db/schema_migrations/20210527134019 | 1 + db/structure.sql | 2 + doc/api/settings.md | 4 +- doc/user/admin_area/diff_limits.md | 27 +++--- lib/gitlab/git/diff_collection.rb | 6 +- locale/gitlab.pot | 14 ++- spec/models/application_setting_spec.rb | 28 ++++++ spec/models/commit_spec.rb | 86 +++++++++++++++++++ spec/requests/api/merge_requests_spec.rb | 2 +- spec/requests/api/settings_spec.rb | 4 + 19 files changed, 243 insertions(+), 31 deletions(-) create mode 100644 config/feature_flags/development/configurable_diff_limits.yml create mode 100644 db/migrate/20210527133919_add_diff_max_lines_to_application_settings.rb create mode 100644 db/migrate/20210527134019_add_diff_max_files_to_application_settings.rb create mode 100644 db/schema_migrations/20210527133919 create mode 100644 db/schema_migrations/20210527134019 diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index 46414b49f37eb5..efdad22fa54c54 100644 --- a/app/helpers/application_settings_helper.rb +++ b/app/helpers/application_settings_helper.rb @@ -338,6 +338,8 @@ def visible_attributes :version_check_enabled, :web_ide_clientside_preview_enabled, :diff_max_patch_bytes, + :diff_max_files, + :diff_max_lines, :commit_email_hostname, :protected_ci_variables, :local_markdown_version, diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 65800e40d6c822..f8047ed9b7848b 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -273,6 +273,18 @@ def self.kroki_formats_attributes 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 :diff_max_files, + presence: true, + numericality: { only_integer: true, + greater_than_or_equal_to: Commit::DEFAULT_MAX_DIFF_FILES_SETTING, + less_than_or_equal_to: Commit::MAX_DIFF_FILES_SETTING_UPPER_BOUND } + + validates :diff_max_lines, + presence: true, + numericality: { only_integer: true, + greater_than_or_equal_to: Commit::DEFAULT_MAX_DIFF_LINES_SETTING, + less_than_or_equal_to: Commit::MAX_DIFF_LINES_SETTING_UPPER_BOUND } + validates :user_default_internal_regex, js_regex: true, allow_nil: true validates :personal_access_token_prefix, diff --git a/app/models/application_setting_implementation.rb b/app/models/application_setting_implementation.rb index bf9df3b9efc119..b613e698471f8e 100644 --- a/app/models/application_setting_implementation.rb +++ b/app/models/application_setting_implementation.rb @@ -60,6 +60,8 @@ def defaults default_projects_limit: Settings.gitlab['default_projects_limit'], default_snippet_visibility: Settings.gitlab.default_projects_features['visibility_level'], diff_max_patch_bytes: Gitlab::Git::Diff::DEFAULT_MAX_PATCH_BYTES, + diff_max_files: Commit::DEFAULT_MAX_DIFF_FILES_SETTING, + diff_max_lines: Commit::DEFAULT_MAX_DIFF_LINES_SETTING, disable_feed_token: false, disabled_oauth_sign_in_sources: [], dns_rebinding_protection_enabled: true, diff --git a/app/models/commit.rb b/app/models/commit.rb index 23c1dffcc63c1d..a1ed5eb9ab9500 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -33,6 +33,12 @@ class Commit # Used by GFM to match and present link extensions on node texts and hrefs. LINK_EXTENSION_PATTERN = /(patch)/.freeze + DEFAULT_MAX_DIFF_LINES_SETTING = 50_000 + DEFAULT_MAX_DIFF_FILES_SETTING = 1_000 + MAX_DIFF_LINES_SETTING_UPPER_BOUND = 100_000 + MAX_DIFF_FILES_SETTING_UPPER_BOUND = 3_000 + DIFF_SAFE_LIMIT_FACTOR = 10 + cache_markdown_field :title, pipeline: :single_line cache_markdown_field :full_title, pipeline: :single_line, limit: 1.kilobyte cache_markdown_field :description, pipeline: :commit_description, limit: 1.megabyte @@ -78,20 +84,24 @@ def truncate_sha(sha) end def diff_safe_lines(project: nil) - Gitlab::Git::DiffCollection.default_limits(project: project)[:max_lines] + diff_safe_max_lines(project: project) end - def diff_hard_limit_files(project: nil) + def diff_max_files(project: nil) if Feature.enabled?(:increased_diff_limits, project) 3000 + elsif Feature.enabled?(:configurable_diff_limits, project) + Gitlab::CurrentSettings.diff_max_files else 1000 end end - def diff_hard_limit_lines(project: nil) + def diff_max_lines(project: nil) if Feature.enabled?(:increased_diff_limits, project) 100000 + elsif Feature.enabled?(:configurable_diff_limits, project) + Gitlab::CurrentSettings.diff_max_lines else 50000 end @@ -99,11 +109,19 @@ def diff_hard_limit_lines(project: nil) def max_diff_options(project: nil) { - max_files: diff_hard_limit_files(project: project), - max_lines: diff_hard_limit_lines(project: project) + max_files: diff_max_files(project: project), + max_lines: diff_max_lines(project: project) } end + def diff_safe_max_files(project: nil) + diff_max_files(project: project) / DIFF_SAFE_LIMIT_FACTOR + end + + def diff_safe_max_lines(project: nil) + diff_max_lines(project: project) / DIFF_SAFE_LIMIT_FACTOR + end + def from_hash(hash, container) raw_commit = Gitlab::Git::Commit.new(container.repository.raw, hash) new(raw_commit, container) diff --git a/app/views/admin/application_settings/_diff_limits.html.haml b/app/views/admin/application_settings/_diff_limits.html.haml index 5351ac5abd1e1c..6a51d2e39d40a6 100644 --- a/app/views/admin/application_settings/_diff_limits.html.haml +++ b/app/views/admin/application_settings/_diff_limits.html.haml @@ -3,11 +3,30 @@ %fieldset .form-group - = f.label :diff_max_patch_bytes, _('Maximum diff patch size in bytes'), class: 'label-light' + = f.label :diff_max_patch_bytes, _('Maximum diff patch size (Bytes)'), class: 'label-light' = f.number_field :diff_max_patch_bytes, class: 'form-control gl-form-input' %span.form-text.text-muted - = _("Collapse diffs larger than this size, and show a 'too large' message instead.") + = _("Diff files surpassing this limit will be presented as 'too large' and won't be expandable.") = link_to sprite_icon('question-o'), - help_page_path('user/admin_area/diff_limits') + help_page_path('user/admin_area/diff_limits', + anchor: 'diff-limits-administration') + + = f.label :diff_max_files, _('Maximum files in a diff'), class: 'label-light' + = f.number_field :diff_max_files, class: 'form-control gl-form-input' + %span.form-text.text-muted + = _("Diff files surpassing this limit will be presented as 'too large' and won't be expandable.") + + = link_to sprite_icon('question-o'), + help_page_path('user/admin_area/diff_limits', + anchor: 'diff-limits-administration') + + = f.label :diff_max_lines, _('Maximum lines in a diff'), class: 'label-light' + = f.number_field :diff_max_lines, class: 'form-control gl-form-input' + %span.form-text.text-muted + = _("Diff files surpassing this limit will be presented as 'too large' and won't be expandable.") + + = link_to sprite_icon('question-o'), + help_page_path('user/admin_area/diff_limits', + anchor: 'diff-limits-administration') = f.submit _('Save changes'), class: 'gl-button btn btn-confirm' diff --git a/config/feature_flags/development/configurable_diff_limits.yml b/config/feature_flags/development/configurable_diff_limits.yml new file mode 100644 index 00000000000000..e73d45fac65edc --- /dev/null +++ b/config/feature_flags/development/configurable_diff_limits.yml @@ -0,0 +1,8 @@ +--- +name: configurable_diff_limits +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/56722 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/332194 +milestone: '14.0' +type: development +group: group::code review +default_enabled: false diff --git a/db/migrate/20210527133919_add_diff_max_lines_to_application_settings.rb b/db/migrate/20210527133919_add_diff_max_lines_to_application_settings.rb new file mode 100644 index 00000000000000..9c1cd94dbaa0e3 --- /dev/null +++ b/db/migrate/20210527133919_add_diff_max_lines_to_application_settings.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class AddDiffMaxLinesToApplicationSettings < ActiveRecord::Migration[6.0] + def change + add_column(:application_settings, + :diff_max_lines, + :integer, + default: 50000, + null: false) + end +end diff --git a/db/migrate/20210527134019_add_diff_max_files_to_application_settings.rb b/db/migrate/20210527134019_add_diff_max_files_to_application_settings.rb new file mode 100644 index 00000000000000..60b1f74cfd0129 --- /dev/null +++ b/db/migrate/20210527134019_add_diff_max_files_to_application_settings.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class AddDiffMaxFilesToApplicationSettings < ActiveRecord::Migration[6.0] + def change + add_column(:application_settings, + :diff_max_files, + :integer, + default: 1000, + null: false) + end +end diff --git a/db/schema_migrations/20210527133919 b/db/schema_migrations/20210527133919 new file mode 100644 index 00000000000000..559860de55dfed --- /dev/null +++ b/db/schema_migrations/20210527133919 @@ -0,0 +1 @@ +aaf5936c945451fa98df7c21ab34c9aa7190dcf301f536c259e5b1fe54407f36 \ No newline at end of file diff --git a/db/schema_migrations/20210527134019 b/db/schema_migrations/20210527134019 new file mode 100644 index 00000000000000..de757dd355e1a0 --- /dev/null +++ b/db/schema_migrations/20210527134019 @@ -0,0 +1 @@ +ac4522ee51d4a4cda317b680c16be3d9ef3e1619bba80c26aefe8d5dc70f013c \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 3c96987bb5cbb8..e3ac9891ad7f04 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -9517,6 +9517,8 @@ CREATE TABLE application_settings ( elasticsearch_username text, encrypted_elasticsearch_password bytea, encrypted_elasticsearch_password_iv bytea, + diff_max_lines integer DEFAULT 50000 NOT NULL, + diff_max_files integer DEFAULT 1000 NOT NULL, CONSTRAINT app_settings_container_reg_cleanup_tags_max_list_size_positive CHECK ((container_registry_cleanup_tags_service_max_list_size >= 0)), CONSTRAINT app_settings_ext_pipeline_validation_service_url_text_limit CHECK ((char_length(external_pipeline_validation_service_url) <= 255)), CONSTRAINT app_settings_registry_exp_policies_worker_capacity_positive CHECK ((container_registry_expiration_policies_worker_capacity >= 0)), diff --git a/doc/api/settings.md b/doc/api/settings.md index 406688732feaa4..3739eabca6f1af 100644 --- a/doc/api/settings.md +++ b/doc/api/settings.md @@ -251,7 +251,9 @@ listed in the descriptions of the relevant settings. | `default_projects_limit` | integer | no | Project limit per user. Default is `100000`. | | `default_snippet_visibility` | string | no | What visibility level new snippets receive. Can take `private`, `internal` and `public` as a parameter. Default is `private`. | | `deletion_adjourned_period` | integer | no | **(PREMIUM SELF)** The number of days to wait before deleting a project or group that is marked for deletion. Value must be between 0 and 90. -| `diff_max_patch_bytes` | integer | no | Maximum diff patch size (Bytes). | +| `diff_max_patch_bytes` | integer | no | Maximum [diff patch size](../user/admin_area/diff_limits.md), in bytes. | +| `diff_max_files` | integer | no | Maximum [files in a diff](../user/admin_area/diff_limits.md). | +| `diff_max_lines` | integer | no | Maximum [lines in a diff](../user/admin_area/diff_limits.md). | | `disable_feed_token` | boolean | no | Disable display of RSS/Atom and calendar feed tokens ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/231493) in GitLab 13.7) | | `disabled_oauth_sign_in_sources` | array of strings | no | Disabled OAuth sign-in sources. | | `dns_rebinding_protection_enabled` | boolean | no | Enforce DNS rebinding attack protection. | diff --git a/doc/user/admin_area/diff_limits.md b/doc/user/admin_area/diff_limits.md index 88d24680500bff..cb4733889fc075 100644 --- a/doc/user/admin_area/diff_limits.md +++ b/doc/user/admin_area/diff_limits.md @@ -12,27 +12,30 @@ You can set a maximum size for display of diff files (patches). For details about diff files, [view changes between files](../project/merge_requests/changes.md). Read more about the [built-in limits for merge requests and diffs](../../administration/instance_limits.md#merge-requests). -## Maximum diff patch size +## Configure diff limits -Diff files which exceed this value are presented as 'too large' and cannot -be expandable. Instead of an expandable view, a link to the blob view is -shown. +WARNING: +These settings are experimental. An increased maximum increases resource +consumption of your instance. Keep this in mind when adjusting the maximum. + +To speed the loading time of merge request views and branch comparison views +on your instance, you can configure three instance-level maximum values for diffs: + +- **Maximum diff patch size**: The total size, in bytes, of the entire diff. +- **Maximum diff files**: The total number of files changed in a diff. +- **Maximum diff lines**: The total number of lines changed in a diff. -Patches greater than 10% of this size are automatically collapsed, and a -link to expand the diff is presented. -This affects merge requests and branch comparison views. +When a diff reaches 10% of any of these values, the files are shown in a +collapsed view, with a link to expand the diff. Diffs that exceed any of the +set values are presented as **Too large** are cannot be expanded in the UI. -To set the maximum diff patch size: +To configure these values: 1. Go to the Admin Area (**{admin}**) and select **Settings > General**. 1. Expand **Diff limits**. 1. Enter a value for **Maximum diff patch size**, measured in bytes. 1. Select **Save changes**. -WARNING: -This setting is experimental. An increased maximum increases resource -consumption of your instance. Keep this in mind when adjusting the maximum. -