Show correct size when MR diff overflows

The problem is that we often go via a diff object constructed from the diffs
stored in the DB. Those diffs, by definition, don't overflow, so we don't have
access to the 'correct' `real_size` - that is stored on the MR diff object
iself.

6 files changed, 52 insertions, 27 deletions

diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 1d4827375d7..9d2288c311e 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -191,22 +191,23 @@ class MergeRequest < ActiveRecord::Base
     merge_request_diff ? merge_request_diff.raw_diffs(*args) : compare.raw_diffs(*args)
   end
 
-  def diffs(diff_options = nil)
+  def diffs(diff_options = {})
     if compare
-      compare.diffs(diff_options)
+      # When saving MR diffs, `no_collapse` is implicitly added (because we need
+      # to save the entire contents to the DB), so add that here for
+      # consistency.
+      compare.diffs(diff_options.merge(no_collapse: true))
     else
       merge_request_diff.diffs(diff_options)
     end
   end
 
   def diff_size
-    # The `#diffs` method ends up at an instance of a class inheriting from
-    # `Gitlab::Diff::FileCollection::Base`, so use those options as defaults
-    # here too, to get the same diff size without performing highlighting.
-    #
-    opts = Gitlab::Diff::FileCollection::Base.default_options.merge(diff_options || {})
+    # Calling `merge_request_diff.diffs.real_size` will also perform
+    # highlighting, which we don't need here.
+    return real_size if merge_request_diff
 
-    raw_diffs(opts).size
+    diffs.real_size
   end
 
   def diff_base_commit
diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb
index 6604af2b47e..f0a3c30ea74 100644
--- a/app/models/merge_request_diff.rb
+++ b/app/models/merge_request_diff.rb
@@ -260,7 +260,7 @@ class MergeRequestDiff < ActiveRecord::Base
       new_attributes[:state] = :empty
     else
       diff_collection = compare.diffs(Commit.max_diff_options)
-      new_attributes[:real_size] = compare.diffs.real_size
+      new_attributes[:real_size] = diff_collection.real_size
 
       if diff_collection.any?
         new_diffs = dump_diffs(diff_collection)
diff --git a/changelogs/unreleased/mr-diff-size-overflow.yml b/changelogs/unreleased/mr-diff-size-overflow.yml
new file mode 100644
index 00000000000..87449930cf2
--- /dev/null
+++ b/changelogs/unreleased/mr-diff-size-overflow.yml
@@ -0,0 +1,4 @@
+---
+title: Show sizes correctly in merge requests when diffs overflow
+merge_request:
+author:
diff --git a/lib/gitlab/diff/file_collection/merge_request_diff.rb b/lib/gitlab/diff/file_collection/merge_request_diff.rb
index 329d12f13d1..0bd226ef050 100644
--- a/lib/gitlab/diff/file_collection/merge_request_diff.rb
+++ b/lib/gitlab/diff/file_collection/merge_request_diff.rb
@@ -15,6 +15,10 @@ module Gitlab
           super.tap { |_| store_highlight_cache }
         end
 
+        def real_size
+          @merge_request_diff.real_size
+        end
+
         private
 
         # Extracted method to highlight in the same iteration to the diff_collection.
diff --git a/spec/features/merge_requests/diffs_spec.rb b/spec/features/merge_requests/diffs_spec.rb
index 32a6a4b2682..7dee3b852ca 100644
--- a/spec/features/merge_requests/diffs_spec.rb
+++ b/spec/features/merge_requests/diffs_spec.rb
@@ -1,13 +1,8 @@
 require 'spec_helper'
 
 feature 'Diffs URL', js: true, feature: true do
-  include ApplicationHelper
-
-  let(:author_user) { create(:user) }
-  let(:user) { create(:user) }
   let(:project) { create(:project, :public) }
-  let(:forked_project) { Projects::ForkService.new(project, author_user).execute }
-  let(:merge_request) { create(:merge_request_with_diffs, source_project: forked_project, target_project: project, author: author_user) }
+  let(:merge_request) { create(:merge_request, source_project: project) }
 
   context 'when visit with */* as accept header' do
     before(:each) do
@@ -27,20 +22,23 @@ feature 'Diffs URL', js: true, feature: true do
 
   context 'when merge request has overflow' do
     it 'displays warning' do
-      allow_any_instance_of(MergeRequestDiff).to receive(:overflow?).and_return(true)
-      allow(Commit).to receive(:max_diff_options).and_return(max_files: 20, max_lines: 20)
+      allow(Commit).to receive(:max_diff_options).and_return(max_files: 3)
 
       visit diffs_namespace_project_merge_request_path(project.namespace, project, merge_request)
 
       page.within('.alert') do
         expect(page).to have_text("Too many changes to show. Plain diff Email patch To preserve
-          performance only 3 of 3 files are displayed.")
+          performance only 3 of 3+ files are displayed.")
       end
     end
   end
 
-  describe 'when editing file' do
-    let(:changelog_id) { hexdigest("CHANGELOG") }
+  context 'when editing file' do
+    let(:author_user) { create(:user) }
+    let(:user) { create(:user) }
+    let(:forked_project) { Projects::ForkService.new(project, author_user).execute }
+    let(:merge_request) { create(:merge_request_with_diffs, source_project: forked_project, target_project: project, author: author_user) }
+    let(:changelog_id) { Digest::SHA1.hexdigest("CHANGELOG") }
 
     context 'as author' do
       it 'shows direct edit link' do
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 415d3e7b200..be08b96641a 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -199,10 +199,10 @@ describe MergeRequest, models: true do
     end
 
     context 'when there are no MR diffs' do
-      it 'delegates to the compare object' do
+      it 'delegates to the compare object, setting no_collapse: true' do
         merge_request.compare = double(:compare)
 
-        expect(merge_request.compare).to receive(:diffs).with(options)
+        expect(merge_request.compare).to receive(:diffs).with(options.merge(no_collapse: true))
 
         merge_request.diffs(options)
       end
@@ -215,15 +215,22 @@ describe MergeRequest, models: true do
     end
 
     context 'when there are MR diffs' do
-      before do
+      it 'returns the correct count' do
         merge_request.save
+
+        expect(merge_request.diff_size).to eq('105')
       end
 
-      it 'returns the correct count' do
-        expect(merge_request.diff_size).to eq(105)
+      it 'returns the correct overflow count' do
+        allow(Commit).to receive(:max_diff_options).and_return(max_files: 2)
+        merge_request.save
+
+        expect(merge_request.diff_size).to eq('2+')
       end
 
       it 'does not perform highlighting' do
+        merge_request.save
+
         expect(Gitlab::Diff::Highlight).not_to receive(:new)
 
         merge_request.diff_size
@@ -231,7 +238,7 @@ describe MergeRequest, models: true do
     end
 
     context 'when there are no MR diffs' do
-      before do
+      def set_compare(merge_request)
         merge_request.compare = CompareService.new(
           merge_request.source_project,
           merge_request.source_branch
@@ -242,10 +249,21 @@ describe MergeRequest, models: true do
       end
 
       it 'returns the correct count' do
-        expect(merge_request.diff_size).to eq(105)
+        set_compare(merge_request)
+
+        expect(merge_request.diff_size).to eq('105')
+      end
+
+      it 'returns the correct overflow count' do
+        allow(Commit).to receive(:max_diff_options).and_return(max_files: 2)
+        set_compare(merge_request)
+
+        expect(merge_request.diff_size).to eq('2+')
       end
 
       it 'does not perform highlighting' do
+        set_compare(merge_request)
+
         expect(Gitlab::Diff::Highlight).not_to receive(:new)
 
         merge_request.diff_size