Merge branch 'sh-limit-commit-renderering' into 'master'
Speed up diff comparisons by limiting number of commit messages rendered See merge request gitlab-org/gitlab-ce!21335
This commit is contained in:
commit
a3382bab11
|
@ -1,4 +1,24 @@
|
||||||
module RendersCommits
|
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)
|
def prepare_commits_for_rendering(commits)
|
||||||
Banzai::CommitRenderer.render(commits, @project, current_user) # rubocop:disable Gitlab/ModuleWithInstanceVariables
|
Banzai::CommitRenderer.render(commits, @project, current_user) # rubocop:disable Gitlab/ModuleWithInstanceVariables
|
||||||
|
|
||||||
|
|
|
@ -63,7 +63,7 @@ class Projects::CommitsController < Projects::ApplicationController
|
||||||
end
|
end
|
||||||
|
|
||||||
@commits = @commits.with_pipeline_status
|
@commits = @commits.with_pipeline_status
|
||||||
@commits = prepare_commits_for_rendering(@commits)
|
@commits = set_commits_for_rendering(@commits)
|
||||||
end
|
end
|
||||||
|
|
||||||
# Rails 5 sets request.format from the extension.
|
# Rails 5 sets request.format from the extension.
|
||||||
|
|
|
@ -78,7 +78,7 @@ class Projects::CompareController < Projects::ApplicationController
|
||||||
end
|
end
|
||||||
|
|
||||||
def define_commits
|
def define_commits
|
||||||
@commits = compare.present? ? prepare_commits_for_rendering(compare.commits) : []
|
@commits = compare.present? ? set_commits_for_rendering(@compare.commits) : []
|
||||||
end
|
end
|
||||||
|
|
||||||
def define_diffs
|
def define_diffs
|
||||||
|
|
|
@ -101,7 +101,7 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap
|
||||||
|
|
||||||
@target_project = @merge_request.target_project
|
@target_project = @merge_request.target_project
|
||||||
@source_project = @merge_request.source_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
|
@commit = @merge_request.diff_head_commit
|
||||||
|
|
||||||
@labels = LabelsFinder.new(current_user, project_id: @project.id).execute
|
@labels = LabelsFinder.new(current_user, project_id: @project.id).execute
|
||||||
|
|
|
@ -79,7 +79,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
|
||||||
# Get commits from repository
|
# Get commits from repository
|
||||||
# or from cache if already merged
|
# or from cache if already merged
|
||||||
@commits =
|
@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') }
|
render json: { html: view_to_html_string('projects/merge_requests/_commits') }
|
||||||
end
|
end
|
||||||
|
|
|
@ -210,17 +210,6 @@ module CommitsHelper
|
||||||
Sanitize.clean(string, remove_contents: true)
|
Sanitize.clean(string, remove_contents: true)
|
||||||
end
|
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)
|
def commit_path(project, commit, merge_request: nil)
|
||||||
if merge_request&.persisted?
|
if merge_request&.persisted?
|
||||||
diffs_project_merge_request_path(project, merge_request, commit_id: commit.id)
|
diffs_project_merge_request_path(project, merge_request, commit_id: commit.id)
|
||||||
|
|
|
@ -1,9 +1,10 @@
|
||||||
- commits, hidden = limited_commits(@commits)
|
- commits = @commits
|
||||||
|
- hidden = @hidden_commit_count
|
||||||
- commits = Commit.decorate(commits, @project)
|
- commits = Commit.decorate(commits, @project)
|
||||||
|
|
||||||
.card
|
.card
|
||||||
.card-header
|
.card-header
|
||||||
Commits (#{@commits.count})
|
Commits (#{@total_commit_count})
|
||||||
- if hidden > 0
|
- if hidden > 0
|
||||||
%ul.content-list
|
%ul.content-list
|
||||||
- commits.each do |commit|
|
- commits.each do |commit|
|
||||||
|
|
|
@ -2,7 +2,8 @@
|
||||||
- project = local_assigns.fetch(:project) { merge_request&.project }
|
- project = local_assigns.fetch(:project) { merge_request&.project }
|
||||||
- ref = local_assigns.fetch(:ref) { merge_request&.source_branch }
|
- 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|
|
- commits.chunk { |c| c.committed_date.in_time_zone.to_date }.each do |day, commits|
|
||||||
%li.commit-header.js-commit-header{ data: { day: day } }
|
%li.commit-header.js-commit-header{ data: { day: day } }
|
||||||
|
|
|
@ -33,7 +33,7 @@
|
||||||
%li.commits-tab.new-tab
|
%li.commits-tab.new-tab
|
||||||
= link_to url_for(safe_params), data: {target: 'div#commits', action: 'new', toggle: 'tabvue'} do
|
= link_to url_for(safe_params), data: {target: 'div#commits', action: 'new', toggle: 'tabvue'} do
|
||||||
Commits
|
Commits
|
||||||
%span.badge.badge-pill= @commits.size
|
%span.badge.badge-pill= @total_commit_count
|
||||||
- if @pipelines.any?
|
- if @pipelines.any?
|
||||||
%li.builds-tab
|
%li.builds-tab
|
||||||
= link_to url_for(safe_params.merge(action: 'pipelines')), data: {target: 'div#pipelines', action: 'pipelines', toggle: 'tabvue'} do
|
= link_to url_for(safe_params.merge(action: 'pipelines')), data: {target: 'div#pipelines', action: 'pipelines', toggle: 'tabvue'} do
|
||||||
|
|
|
@ -0,0 +1,5 @@
|
||||||
|
---
|
||||||
|
title: Speed up diff comparisons by limiting number of commit messages rendered
|
||||||
|
merge_request: 21335
|
||||||
|
author:
|
||||||
|
type: performance
|
|
@ -29,6 +29,55 @@ describe Projects::MergeRequests::CreationsController do
|
||||||
expect(response).to be_success
|
expect(response).to be_success
|
||||||
end
|
end
|
||||||
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
|
end
|
||||||
|
|
||||||
describe 'GET diffs' do
|
describe 'GET diffs' do
|
||||||
|
|
|
@ -20,6 +20,7 @@ describe 'projects/merge_requests/_commits.html.haml' do
|
||||||
|
|
||||||
assign(:merge_request, merge_request)
|
assign(:merge_request, merge_request)
|
||||||
assign(:commits, merge_request.commits)
|
assign(:commits, merge_request.commits)
|
||||||
|
assign(:hidden_commit_count, 0)
|
||||||
end
|
end
|
||||||
|
|
||||||
it 'shows commits from source project' do
|
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)
|
expect(rendered).to have_link(href: href)
|
||||||
end
|
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
|
end
|
||||||
|
|
|
@ -9,6 +9,8 @@ describe 'projects/merge_requests/creations/_new_submit.html.haml' do
|
||||||
|
|
||||||
assign(:merge_request, merge_request)
|
assign(:merge_request, merge_request)
|
||||||
assign(:commits, merge_request.commits)
|
assign(:commits, merge_request.commits)
|
||||||
|
assign(:hidden_commit_count, 0)
|
||||||
|
assign(:total_commit_count, merge_request.commits.count)
|
||||||
assign(:project, merge_request.target_project)
|
assign(:project, merge_request.target_project)
|
||||||
|
|
||||||
allow(view).to receive(:can?).and_return(true)
|
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')
|
expect(rendered).not_to have_text('Builds')
|
||||||
end
|
end
|
||||||
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
|
end
|
||||||
|
|
Loading…
Reference in New Issue