Collapsed diffs lines/size don't acumulate to overflow diffs.19820-tweak-single-diff-file-config

10 files changed, 64 insertions, 20 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 19286abd74f..24b4bf4aaec 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -84,6 +84,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 ee23f712f05..8d62cb7d7c9 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.2', git: 'https://gitlab.com/gitlab-org/gitlab_git', branch: 'diff-collection-collapsable'
 
 # LDAP Auth
 # GitLab fork with several improvements to original library. For full list of changes
diff --git a/Gemfile.lock b/Gemfile.lock
index 67c0645c3d9..18f3879cff7 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -1,3 +1,14 @@
+GIT
+  remote: https://gitlab.com/gitlab-org/gitlab_git
+  revision: 4c10fe78053e9044156f96f10895af1ee703eb7b
+  branch: diff-collection-collapsable
+  specs:
+    gitlab_git (10.3.0)
+      activesupport (~> 4.0)
+      charlock_holmes (~> 0.7.3)
+      github-linguist (~> 4.7.0)
+      rugged (~> 0.24.0)
+
 GEM
   remote: https://rubygems.org/
   specs:
@@ -274,11 +285,6 @@ GEM
       diff-lcs (~> 1.1)
       mime-types (>= 1.16, < 3)
       posix-spawn (~> 0.3)
-    gitlab_git (10.2.3)
-      activesupport (~> 4.0)
-      charlock_holmes (~> 0.7.3)
-      github-linguist (~> 4.7.0)
-      rugged (~> 0.24.0)
     gitlab_meta (7.0)
     gitlab_omniauth-ldap (1.2.1)
       net-ldap (~> 0.9)
@@ -861,7 +867,7 @@ DEPENDENCIES
   github-linguist (~> 4.7.0)
   github-markup (~> 1.3.1)
   gitlab-flowdock-git-hook (~> 1.0.1)
-  gitlab_git (~> 10.2)
+  gitlab_git (~> 10.2)!
   gitlab_meta (= 7.0)
   gitlab_omniauth-ldap (~> 1.2.1)
   gollum-lib (~> 4.1.0)
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 b38e0180d44..7bcfa029b77 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,17 +23,18 @@ module DiffHelper
   end
 
   def diff_hard_limit_enabled?
-    params[:force_show_diff].present?
+    params[:force_show_diff].present? || expand_all_diffs?
   end
 
   def diff_options
-    options = { ignore_whitespace_change: hide_whitespace? }
+    options = { ignore_whitespace_change: hide_whitespace?, no_collapse: expand_all_diffs? }
 
     if diff_hard_limit_enabled?
       options.merge!(Commit.max_diff_options)
     end
 
     if action_name == 'diff_for_path'
+      options[:no_collapse] = true
       options[:paths] = params.values_at(:old_path, :new_path)
     end
 
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..8c14500eedd 100644
--- a/lib/gitlab/diff/file.rb
+++ b/lib/gitlab/diff/file.rb
@@ -68,8 +68,12 @@ 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
+      def collapsed?
+        diff.collapsed?
+      end
+
+      def collapse_by_default?
+        diff.collapse?
       end
 
       def highlighted_diff_lines
diff --git a/spec/features/expand_collapse_diffs_spec.rb b/spec/features/expand_collapse_diffs_spec.rb
index 78bc888f2a6..32c49616532 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,14 @@ 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 '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 7ea9fa933f0..f16bc79033b 100644
--- a/spec/helpers/diff_helper_spec.rb
+++ b/spec/helpers/diff_helper_spec.rb
@@ -37,6 +37,11 @@ describe DiffHelper do
       expect(diff_hard_limit_enabled?).to be_truthy
     end
 
+    it 'should return true if param is provided to give user more diff space' do
+      allow(controller).to receive(:params) { { expand_all_diffs: 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
@@ -48,9 +53,28 @@ describe DiffHelper do
       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 safe limit for a diff if force diff is false' do
       expect(diff_options).not_to include(:max_lines, :max_files)
     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