diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 1d4827375d70f955c1dbef0c49849981456f7eb8..9d2288c311e6784ecef2fd2c1e4fd3cb425bb2da 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 6604af2b47edd8f9d7fcac510f206b09235523cf..f0a3c30ea741996cb94307780f45c24dc71ca3ae 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 0000000000000000000000000000000000000000..87449930cf23a3d35bd2e53e7ed0f6e2939edf7b --- /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 329d12f13d161210356d1d6342c01b2ca77b85c1..0bd226ef0504c265bc084506be024a7327a52f2a 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 32a6a4b26820bb7a62a0b84250a54d5351699798..7dee3b852ca6f4777942172f3d1d60fa90243f84 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 415d3e7b2004ecb3b797c6625d0334a66c882095..be08b96641a6df3a69d583bdc51e70b5225cb366 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