Merge branch '19820-safer-diffs' into 'master'

Collapsed diffs lines/size don't accumulate to overflow diffs.

## What does this MR do?

Reduce the number of lines that we highlight on big diffs to try to reduce the degradation introduced by  !4990 as you can see on the issue description #19820 .

We collapse any files after the diff is over the number of safe files (100), or over the safe lines (500) or over the safe number of bytes (5KB x 100 files).

We still need to bump the gitlab_git and point this branch to the new gitlab_git version.

## What are the relevant issue numbers?

Closes #19820 
Closes #19885

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- ~~[ ] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)~~
- ~~[ ] API support added~~
- Tests
  - [x] Added for this feature/bug
  - [x] All builds are passing
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)


See merge request !5306

12 files changed, 96 insertions, 23 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 7bc020c8d68..1f778c69c96 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -103,6 +103,7 @@ v 8.10.0 (unreleased)
   - Add min value for project limit field on user's form !3622 (jastkand)
   - Reset project pushes_since_gc when we enqueue the git gc call
   - Add reminder to not paste private SSH keys !4399 (Ingo Blechschmidt)
+  - Collapsed diffs lines/size don't acumulate to overflow diffs.
   - Remove duplicate `description` field in `MergeRequest` entities (Ben Boeckel)
   - Style of import project buttons were fixed in the new project page. !5183 (rdemirbay)
   - Fix GitHub client requests when rate limit is disabled
diff --git a/Gemfile b/Gemfile
index 81e8ff60ad5..6ae9086a541 100644
--- a/Gemfile
+++ b/Gemfile
@@ -52,7 +52,7 @@ gem 'browser', '~> 2.2'
 
 # Extracting information from a git repository
 # Provide access to Gitlab::Git library
-gem 'gitlab_git', '~> 10.2'
+gem 'gitlab_git', '~> 10.3.2'
 
 # LDAP Auth
 # GitLab fork with several improvements to original library. For full list of changes
diff --git a/Gemfile.lock b/Gemfile.lock
index 0987fd5665a..9ec5fe12820 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -274,7 +274,7 @@ GEM
       diff-lcs (~> 1.1)
       mime-types (>= 1.16, < 3)
       posix-spawn (~> 0.3)
-    gitlab_git (10.2.3)
+    gitlab_git (10.3.2)
       activesupport (~> 4.0)
       charlock_holmes (~> 0.7.3)
       github-linguist (~> 4.7.0)
@@ -861,7 +861,7 @@ DEPENDENCIES
   github-linguist (~> 4.7.0)
   github-markup (~> 1.4)
   gitlab-flowdock-git-hook (~> 1.0.1)
-  gitlab_git (~> 10.2)
+  gitlab_git (~> 10.3.2)
   gitlab_meta (= 7.0)
   gitlab_omniauth-ldap (~> 1.2.1)
   gollum-lib (~> 4.2)
diff --git a/app/controllers/concerns/diff_for_path.rb b/app/controllers/concerns/diff_for_path.rb
index e09b8789eb2..026d8b2e1e0 100644
--- a/app/controllers/concerns/diff_for_path.rb
+++ b/app/controllers/concerns/diff_for_path.rb
@@ -10,7 +10,6 @@ module DiffForPath
 
     diff_commit = commit_for_diff(diff_file)
     blob = diff_file.blob(diff_commit)
-    @expand_all_diffs = true
 
     locals = {
       diff_file: diff_file,
diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb
index adab901700c..75b029365f9 100644
--- a/app/helpers/diff_helper.rb
+++ b/app/helpers/diff_helper.rb
@@ -9,7 +9,7 @@ module DiffHelper
   end
 
   def expand_all_diffs?
-    @expand_all_diffs || params[:expand_all_diffs].present?
+    params[:expand_all_diffs].present?
   end
 
   def diff_view
@@ -23,13 +23,14 @@ module DiffHelper
   end
 
   def diff_options
-    default_options = Commit.max_diff_options
+    options = { ignore_whitespace_change: hide_whitespace?, no_collapse: expand_all_diffs? }
 
     if action_name == 'diff_for_path'
-      default_options[:paths] = params.values_at(:old_path, :new_path)
+      options[:no_collapse] = true
+      options[:paths] = params.values_at(:old_path, :new_path)
     end
 
-    default_options.merge(ignore_whitespace_change: hide_whitespace?)
+    Commit.max_diff_options.merge(options)
   end
 
   def safe_diff_files(diffs, diff_refs: nil, repository: nil)
diff --git a/app/views/projects/diffs/_content.html.haml b/app/views/projects/diffs/_content.html.haml
index 0c0424edffd..a1b071f130c 100644
--- a/app/views/projects/diffs/_content.html.haml
+++ b/app/views/projects/diffs/_content.html.haml
@@ -8,12 +8,12 @@
   - elsif blob_text_viewable?(blob)
     - if !project.repository.diffable?(blob)
       .nothing-here-block This diff was suppressed by a .gitattributes entry.
+    - elsif diff_file.collapsed?
+      - url = url_for(params.merge(action: :diff_for_path, old_path: diff_file.old_path, new_path: diff_file.new_path))
+      .nothing-here-block.diff-collapsed{data: { diff_for_path: url } }
+        This diff is collapsed. Click to expand it.
     - elsif diff_file.diff_lines.length > 0
-      - if diff_file.collapsed_by_default? && !expand_all_diffs?
-        - url = url_for(params.merge(action: :diff_for_path, old_path: diff_file.old_path, new_path: diff_file.new_path))
-        .nothing-here-block.diff-collapsed{data: { diff_for_path: url } }
-          This diff is collapsed. Click to expand it.
-      - elsif diff_view == 'parallel'
+      - if diff_view == 'parallel'
         = render "projects/diffs/parallel_view", diff_file: diff_file, project: project, blob: blob
       - else
         = render "projects/diffs/text_file", diff_file: diff_file
diff --git a/app/views/projects/diffs/_diffs.html.haml b/app/views/projects/diffs/_diffs.html.haml
index 20aaab5accf..8ae433b4823 100644
--- a/app/views/projects/diffs/_diffs.html.haml
+++ b/app/views/projects/diffs/_diffs.html.haml
@@ -6,7 +6,7 @@
 
 .content-block.oneline-block.files-changed
   .inline-parallel-buttons
-    - unless expand_all_diffs?
+    - 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: 'html')), class: 'btn btn-default'
     - if show_whitespace_toggle
       - if current_controller?(:commit)
diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb
index 7e01f7b61fb..b09ca1fb8b0 100644
--- a/lib/gitlab/diff/file.rb
+++ b/lib/gitlab/diff/file.rb
@@ -5,7 +5,7 @@ module Gitlab
 
       delegate :new_file, :deleted_file, :renamed_file,
         :old_path, :new_path, :a_mode, :b_mode,
-        :submodule?, :too_large?, to: :diff, prefix: false
+        :submodule?, :too_large?, :collapsed?, to: :diff, prefix: false
 
       def initialize(diff, repository:, diff_refs: nil)
         @diff = diff
@@ -68,10 +68,6 @@ module Gitlab
         @lines ||= Gitlab::Diff::Parser.new.parse(raw_diff.each_line).to_a
       end
 
-      def collapsed_by_default?
-        diff.diff.bytesize > 10240 # 10 KB
-      end
-
       def highlighted_diff_lines
         @highlighted_diff_lines ||= Gitlab::Diff::Highlight.new(self, repository: self.repository).highlight
       end
diff --git a/spec/features/expand_collapse_diffs_spec.rb b/spec/features/expand_collapse_diffs_spec.rb
index 78bc888f2a6..688f68d3cff 100644
--- a/spec/features/expand_collapse_diffs_spec.rb
+++ b/spec/features/expand_collapse_diffs_spec.rb
@@ -3,10 +3,11 @@ require 'spec_helper'
 feature 'Expand and collapse diffs', js: true, feature: true do
   include WaitForAjax
 
+  let(:branch) { 'expand-collapse-diffs' }
+
   before do
     login_as :admin
     project = create(:project)
-    branch = 'expand-collapse-diffs'
 
     # Ensure that undiffable.md is in .gitattributes
     project.repository.copy_gitattributes(branch)
@@ -167,6 +168,46 @@ feature 'Expand and collapse diffs', js: true, feature: true do
     end
   end
 
+  context 'visiting a commit without collapsed diffs' do
+    let(:branch) { 'feature' }
+
+    it 'does not show Expand all button' do
+      expect(page).not_to have_link('Expand all')
+    end
+  end
+
+  context 'visiting a commit with more than safe files' do
+    let(:branch) { 'expand-collapse-files' }
+
+    # safe-files -> 100 | safe-lines -> 5000 | commit-files -> 105
+    it 'does collapsing from the safe number of files to the end on small files' do
+      expect(page).to have_link('Expand all')
+
+      expect(page).to have_selector('.diff-content', count: 105)
+      expect(page).to have_selector('.diff-collapsed', count: 5)
+
+      %w(file-95.txt file-96.txt file-97.txt file-98.txt file-99.txt).each do |filename|
+        expect(find("[data-blob-diff-path*='#{filename}']")).to have_selector('.diff-collapsed')
+      end
+    end
+  end
+
+  context 'visiting a commit with more than safe lines' do
+    let(:branch) { 'expand-collapse-lines' }
+
+    # safe-files -> 100 | safe-lines -> 5000 | commit_files -> 8 (each 1250 lines)
+    it 'does collapsing from the safe number of lines to the end' do
+      expect(page).to have_link('Expand all')
+
+      expect(page).to have_selector('.diff-content', count: 6)
+      expect(page).to have_selector('.diff-collapsed', count: 2)
+
+      %w(file-4.txt file-5.txt).each do |filename|
+        expect(find("[data-blob-diff-path*='#{filename}']")).to have_selector('.diff-collapsed')
+      end
+    end
+  end
+
   context 'expanding all diffs' do
     before do
       click_link('Expand all')
diff --git a/spec/helpers/diff_helper_spec.rb b/spec/helpers/diff_helper_spec.rb
index 4b134a48410..c2fd2c8a533 100644
--- a/spec/helpers/diff_helper_spec.rb
+++ b/spec/helpers/diff_helper_spec.rb
@@ -30,12 +30,31 @@ describe DiffHelper do
       expect(helper.diff_view).to eq 'inline'
     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 hard limit for a diff if expand_all_diffs is true' do
+      allow(controller).to receive(:params) { { expand_all_diffs: true } }
+      expect(diff_options).to include(Commit.max_diff_options)
+    end
+
+    it 'should return no collapse false' do
+      expect(diff_options).to include(no_collapse: false)
+    end
+
+    it 'should return 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)
+    end
+
+    it 'should return 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)
+    end
   end
 
   describe 'unfold_bottom_class' do
diff --git a/spec/lib/gitlab/diff/file_spec.rb b/spec/lib/gitlab/diff/file_spec.rb
index 0460dcf4658..e883a6eb9c2 100644
--- a/spec/lib/gitlab/diff/file_spec.rb
+++ b/spec/lib/gitlab/diff/file_spec.rb
@@ -32,4 +32,18 @@ describe Gitlab::Diff::File, lib: true do
       expect(diff_file.too_large?).to eq(false)
     end
   end
+
+  describe '#collapsed?' do
+    it 'returns true for a file that is quite big' do
+      expect(diff).to receive(:collapsed?).and_return(true)
+
+      expect(diff_file.collapsed?).to eq(true)
+    end
+
+    it 'returns false for a file that is small enough' do
+      expect(diff).to receive(:collapsed?).and_return(false)
+
+      expect(diff_file.collapsed?).to eq(false)
+    end
+  end
 end
diff --git a/spec/support/test_env.rb b/spec/support/test_env.rb
index bb6c84262f6..83f2ad96fd8 100644
--- a/spec/support/test_env.rb
+++ b/spec/support/test_env.rb
@@ -18,7 +18,9 @@ module TestEnv
     'orphaned-branch'       => '45127a9',
     'binary-encoding'       => '7b1cf43',
     'gitattributes'         => '5a62481',
-    'expand-collapse-diffs' => '4842455'
+    'expand-collapse-diffs' => '4842455',
+    'expand-collapse-files' => '025db92',
+    'expand-collapse-lines' => '238e82d'
   }
 
   # gitlab-test-fork is a fork of gitlab-fork, but we don't necessarily