diff options
13 files changed, 112 insertions, 19 deletions
diff --git a/app/controllers/concerns/renders_commits.rb b/app/controllers/concerns/renders_commits.rb index fb41dc1e8a8..b1c9b1e532f 100644 --- a/app/controllers/concerns/renders_commits.rb +++ b/app/controllers/concerns/renders_commits.rb @@ -1,4 +1,24 @@ module RendersCommits + def limited_commits(commits) + if commits.size > MergeRequestDiff::COMMITS_SAFE_SIZE + [ + commits.first(MergeRequestDiff::COMMITS_SAFE_SIZE), + commits.size - MergeRequestDiff::COMMITS_SAFE_SIZE + ] + else + [commits, 0] + end + end + + # This is used as a helper method in a controller. + # rubocop: disable Gitlab/ModuleWithInstanceVariables + def set_commits_for_rendering(commits) + @total_commit_count = commits.size + limited, @hidden_commit_count = limited_commits(commits) + prepare_commits_for_rendering(limited) + end + # rubocop: enable Gitlab/ModuleWithInstanceVariables + def prepare_commits_for_rendering(commits) Banzai::CommitRenderer.render(commits, @project, current_user) # rubocop:disable Gitlab/ModuleWithInstanceVariables diff --git a/app/controllers/projects/commits_controller.rb b/app/controllers/projects/commits_controller.rb index 36faea8056e..5546bef850b 100644 --- a/app/controllers/projects/commits_controller.rb +++ b/app/controllers/projects/commits_controller.rb @@ -63,7 +63,7 @@ class Projects::CommitsController < Projects::ApplicationController end @commits = @commits.with_pipeline_status - @commits = prepare_commits_for_rendering(@commits) + @commits = set_commits_for_rendering(@commits) end # Rails 5 sets request.format from the extension. diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb index f93e500a07a..a1e12821caf 100644 --- a/app/controllers/projects/compare_controller.rb +++ b/app/controllers/projects/compare_controller.rb @@ -78,7 +78,7 @@ class Projects::CompareController < Projects::ApplicationController end def define_commits - @commits = compare.present? ? prepare_commits_for_rendering(compare.commits) : [] + @commits = compare.present? ? set_commits_for_rendering(@compare.commits) : [] end def define_diffs diff --git a/app/controllers/projects/merge_requests/creations_controller.rb b/app/controllers/projects/merge_requests/creations_controller.rb index 81129456ad8..03d0290ac1d 100644 --- a/app/controllers/projects/merge_requests/creations_controller.rb +++ b/app/controllers/projects/merge_requests/creations_controller.rb @@ -101,7 +101,7 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap @target_project = @merge_request.target_project @source_project = @merge_request.source_project - @commits = prepare_commits_for_rendering(@merge_request.commits) + @commits = set_commits_for_rendering(@merge_request.commits) @commit = @merge_request.diff_head_commit @labels = LabelsFinder.new(current_user, project_id: @project.id).execute diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 1b069fe507b..d31b58972ca 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -79,7 +79,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo # Get commits from repository # or from cache if already merged @commits = - prepare_commits_for_rendering(@merge_request.commits.with_pipeline_status) + set_commits_for_rendering(@merge_request.commits.with_pipeline_status) render json: { html: view_to_html_string('projects/merge_requests/_commits') } end diff --git a/app/helpers/commits_helper.rb b/app/helpers/commits_helper.rb index 89fe90fd801..7a942c44ac4 100644 --- a/app/helpers/commits_helper.rb +++ b/app/helpers/commits_helper.rb @@ -210,17 +210,6 @@ module CommitsHelper Sanitize.clean(string, remove_contents: true) end - def limited_commits(commits) - if commits.size > MergeRequestDiff::COMMITS_SAFE_SIZE - [ - commits.first(MergeRequestDiff::COMMITS_SAFE_SIZE), - commits.size - MergeRequestDiff::COMMITS_SAFE_SIZE - ] - else - [commits, 0] - end - end - def commit_path(project, commit, merge_request: nil) if merge_request&.persisted? diffs_project_merge_request_path(project, merge_request, commit_id: commit.id) diff --git a/app/views/projects/commits/_commit_list.html.haml b/app/views/projects/commits/_commit_list.html.haml index 8f8eb2c3d5a..6ed65d07202 100644 --- a/app/views/projects/commits/_commit_list.html.haml +++ b/app/views/projects/commits/_commit_list.html.haml @@ -1,9 +1,10 @@ -- commits, hidden = limited_commits(@commits) +- commits = @commits +- hidden = @hidden_commit_count - commits = Commit.decorate(commits, @project) .card .card-header - Commits (#{@commits.count}) + Commits (#{@total_commit_count}) - if hidden > 0 %ul.content-list - commits.each do |commit| diff --git a/app/views/projects/commits/_commits.html.haml b/app/views/projects/commits/_commits.html.haml index ac6852751be..ec05ff50f25 100644 --- a/app/views/projects/commits/_commits.html.haml +++ b/app/views/projects/commits/_commits.html.haml @@ -2,7 +2,8 @@ - project = local_assigns.fetch(:project) { merge_request&.project } - ref = local_assigns.fetch(:ref) { merge_request&.source_branch } -- commits, hidden = limited_commits(@commits) +- commits = @commits +- hidden = @hidden_commit_count - commits.chunk { |c| c.committed_date.in_time_zone.to_date }.each do |day, commits| %li.commit-header.js-commit-header{ data: { day: day } } diff --git a/app/views/projects/merge_requests/creations/_new_submit.html.haml b/app/views/projects/merge_requests/creations/_new_submit.html.haml index f7a5d85500f..d5c4134dee2 100644 --- a/app/views/projects/merge_requests/creations/_new_submit.html.haml +++ b/app/views/projects/merge_requests/creations/_new_submit.html.haml @@ -33,7 +33,7 @@ %li.commits-tab.new-tab = link_to url_for(safe_params), data: {target: 'div#commits', action: 'new', toggle: 'tabvue'} do Commits - %span.badge.badge-pill= @commits.size + %span.badge.badge-pill= @total_commit_count - if @pipelines.any? %li.builds-tab = link_to url_for(safe_params.merge(action: 'pipelines')), data: {target: 'div#pipelines', action: 'pipelines', toggle: 'tabvue'} do diff --git a/changelogs/unreleased/sh-limit-commit-renderering.yml b/changelogs/unreleased/sh-limit-commit-renderering.yml new file mode 100644 index 00000000000..c44c67bcc90 --- /dev/null +++ b/changelogs/unreleased/sh-limit-commit-renderering.yml @@ -0,0 +1,5 @@ +--- +title: Speed up diff comparisons by limiting number of commit messages rendered +merge_request: 21335 +author: +type: performance diff --git a/spec/controllers/projects/merge_requests/creations_controller_spec.rb b/spec/controllers/projects/merge_requests/creations_controller_spec.rb index d8995f98575..f8c37c0a676 100644 --- a/spec/controllers/projects/merge_requests/creations_controller_spec.rb +++ b/spec/controllers/projects/merge_requests/creations_controller_spec.rb @@ -29,6 +29,55 @@ describe Projects::MergeRequests::CreationsController do expect(response).to be_success end end + + context 'merge request with some commits' do + render_views + + let(:large_diff_params) do + { + namespace_id: fork_project.namespace.to_param, + project_id: fork_project, + merge_request: { + source_branch: 'master', + target_branch: 'fix' + } + } + end + + describe 'with artificial limits' do + before do + # Load MergeRequestdiff so stub_const won't override it with its own definition + # See https://github.com/rspec/rspec-mocks/issues/1079 + stub_const("#{MergeRequestDiff}::COMMITS_SAFE_SIZE", 2) + end + + it 'limits total commits' do + get :new, large_diff_params + + expect(response).to be_success + + total = assigns(:total_commit_count) + expect(assigns(:commits)).to be_an Array + expect(total).to be > 0 + expect(assigns(:hidden_commit_count)).to be > 0 + expect(response).to have_gitlab_http_status(200) + expect(response.body).to match %r(<span class="commits-count">2 commits</span>) + end + end + + it 'shows total commits' do + get :new, large_diff_params + + expect(response).to be_success + + total = assigns(:total_commit_count) + expect(assigns(:commits)).to be_an Array + expect(total).to be > 0 + expect(assigns(:hidden_commit_count)).to eq(0) + expect(response).to have_gitlab_http_status(200) + expect(response.body).to match %r(<span class="commits-count">#{total} commits</span>) + end + end end describe 'GET diffs' do diff --git a/spec/views/projects/merge_requests/_commits.html.haml_spec.rb b/spec/views/projects/merge_requests/_commits.html.haml_spec.rb index b1c6565c08a..a7628548de6 100644 --- a/spec/views/projects/merge_requests/_commits.html.haml_spec.rb +++ b/spec/views/projects/merge_requests/_commits.html.haml_spec.rb @@ -20,6 +20,7 @@ describe 'projects/merge_requests/_commits.html.haml' do assign(:merge_request, merge_request) assign(:commits, merge_request.commits) + assign(:hidden_commit_count, 0) end it 'shows commits from source project' do @@ -30,4 +31,16 @@ describe 'projects/merge_requests/_commits.html.haml' do expect(rendered).to have_link(href: href) end + + context 'when there are hidden commits' do + before do + assign(:hidden_commit_count, 1) + end + + it 'shows notice about omitted commits' do + render + + expect(rendered).to match(/1 additional commit has been omitted to prevent performance issues/) + end + end end diff --git a/spec/views/projects/merge_requests/creations/_new_submit.html.haml_spec.rb b/spec/views/projects/merge_requests/creations/_new_submit.html.haml_spec.rb index 9ab105c3238..8befae39d3a 100644 --- a/spec/views/projects/merge_requests/creations/_new_submit.html.haml_spec.rb +++ b/spec/views/projects/merge_requests/creations/_new_submit.html.haml_spec.rb @@ -9,6 +9,8 @@ describe 'projects/merge_requests/creations/_new_submit.html.haml' do assign(:merge_request, merge_request) assign(:commits, merge_request.commits) + assign(:hidden_commit_count, 0) + assign(:total_commit_count, merge_request.commits.count) assign(:project, merge_request.target_project) allow(view).to receive(:can?).and_return(true) @@ -29,4 +31,17 @@ describe 'projects/merge_requests/creations/_new_submit.html.haml' do expect(rendered).not_to have_text('Builds') end end + + context 'when there are hidden commits' do + before do + assign(:pipelines, Ci::Pipeline.none) + assign(:hidden_commit_count, 2) + end + + it 'shows notice about omitted commits' do + render + + expect(rendered).to match(/2 additional commits have been omitted to prevent performance issues/) + end + end end |