Rollback to use safe diff options19820-rollback-to-safe-limits

6 files changed, 56 insertions, 4 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 863fa4df705..d969648bcfb 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -75,6 +75,7 @@ v 8.10.0 (unreleased)
   - Set import_url validation to be more strict
   - Memoize MR merged/closed events retrieval
   - Don't render discussion notes when requesting diff tab through AJAX
+  - Rollback to safe diff limits to keep performance.
   - Add basic system information like memory and disk usage to the admin panel
   - Don't garbage collect commits that have related DB records like comments
   - More descriptive message for git hooks and file locks
diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb
index adab901700c..b38e0180d44 100644
--- a/app/helpers/diff_helper.rb
+++ b/app/helpers/diff_helper.rb
@@ -22,14 +22,22 @@ module DiffHelper
     end
   end
 
+  def diff_hard_limit_enabled?
+    params[:force_show_diff].present?
+  end
+
   def diff_options
-    default_options = Commit.max_diff_options
+    options = { ignore_whitespace_change: hide_whitespace? }
+
+    if diff_hard_limit_enabled?
+      options.merge!(Commit.max_diff_options)
+    end
 
     if action_name == 'diff_for_path'
-      default_options[:paths] = params.values_at(:old_path, :new_path)
+      options[:paths] = params.values_at(:old_path, :new_path)
     end
 
-    default_options.merge(ignore_whitespace_change: hide_whitespace?)
+    options
   end
 
   def safe_diff_files(diffs, diff_refs: nil, repository: nil)
diff --git a/app/views/projects/diffs/_warning.html.haml b/app/views/projects/diffs/_warning.html.haml
index 10fa1ddf2e5..15536c17f8e 100644
--- a/app/views/projects/diffs/_warning.html.haml
+++ b/app/views/projects/diffs/_warning.html.haml
@@ -2,6 +2,9 @@
   %h4
     Too many changes to show.
     .pull-right
+      - unless diff_hard_limit_enabled?
+        = link_to "Reload with full diff", url_for(params.merge(force_show_diff: true, format: nil)), class: "btn btn-sm"
+
       - if current_controller?(:commit) or current_controller?(:merge_requests)
         - if current_controller?(:commit)
           = link_to "Plain diff", namespace_project_commit_path(@project.namespace, @project, @commit, format: :diff), class: "btn btn-sm"
diff --git a/features/project/commits/commits.feature b/features/project/commits/commits.feature
index 8b0cb90765e..278ce4d78b4 100644
--- a/features/project/commits/commits.feature
+++ b/features/project/commits/commits.feature
@@ -87,6 +87,11 @@ Feature: Project Commits
     Given I visit a commit with an image that changed
     Then The diff links to both the previous and current image
 
+  Scenario: I browse big commit
+    Given I visit big commit page
+    Then I see big commit warning
+    And I see "Reload with full diff" link
+
   @javascript
   Scenario: I filter commits by message
     When I search "submodules" commits
diff --git a/features/steps/project/commits/commits.rb b/features/steps/project/commits/commits.rb
index bea9f9d198b..174f27e3c8e 100644
--- a/features/steps/project/commits/commits.rb
+++ b/features/steps/project/commits/commits.rb
@@ -125,6 +125,26 @@ class Spinach::Features::ProjectCommits < Spinach::FeatureSteps
     expect(page).to have_content 'Authors'
   end
 
+  step 'I visit big commit page' do
+
+    # Create a temporary scope to ensure that the stub_const is removed after user
+    RSpec::Mocks.with_temporary_scope do
+      stub_const('Gitlab::Git::DiffCollection::DEFAULT_LIMITS', { max_lines: 1, max_files: 1 })
+      visit namespace_project_commit_path(@project.namespace, @project, sample_big_commit.id)
+    end
+  end
+
+  step 'I see big commit warning' do
+    expect(page).to have_content sample_big_commit.message
+    expect(page).to have_content "Too many changes"
+  end
+
+  step 'I see "Reload with full diff" link' do
+    link = find_link('Reload with full diff')
+    expect(link[:href]).to end_with('?force_show_diff=true')
+    expect(link[:href]).not_to include('.html')
+  end
+
   step 'I visit a commit with an image that changed' do
     visit namespace_project_commit_path(@project.namespace, @project, sample_image_commit.id)
   end
diff --git a/spec/helpers/diff_helper_spec.rb b/spec/helpers/diff_helper_spec.rb
index 4b134a48410..7ea9fa933f0 100644
--- a/spec/helpers/diff_helper_spec.rb
+++ b/spec/helpers/diff_helper_spec.rb
@@ -31,11 +31,26 @@ describe DiffHelper do
     end
   end
 
+  describe 'diff_hard_limit_enabled?' do
+    it 'should return true if param is provided' do
+      allow(controller).to receive(:params) { { force_show_diff: true } }
+      expect(diff_hard_limit_enabled?).to be_truthy
+    end
+
+    it 'should return false if param is not provided' do
+      expect(diff_hard_limit_enabled?).to be_falsey
+    end
+  end
+
   describe 'diff_options' do
-    it 'should return hard limit for a diff' do
+    it 'should return hard limit for a diff if force diff is true' do
       allow(controller).to receive(:params) { { force_show_diff: true } }
       expect(diff_options).to include(Commit.max_diff_options)
     end
+
+    it 'should return safe limit for a diff if force diff is false' do
+      expect(diff_options).not_to include(:max_lines, :max_files)
+    end
   end
 
   describe 'unfold_bottom_class' do