From a0979c05fd5976cabe3c0633c168848d66320bfa Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Thu, 20 Apr 2017 15:47:32 +0100 Subject: [PATCH] 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. --- app/models/merge_request.rb | 17 +++++----- app/models/merge_request_diff.rb | 2 +- .../unreleased/mr-diff-size-overflow.yml | 4 +++ .../file_collection/merge_request_diff.rb | 4 +++ spec/features/merge_requests/diffs_spec.rb | 20 ++++++------ spec/models/merge_request_spec.rb | 32 +++++++++++++++---- 6 files changed, 52 insertions(+), 27 deletions(-) create mode 100644 changelogs/unreleased/mr-diff-size-overflow.yml 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 -- GitLab