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

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