Read about our upcoming Code of Conduct on this issue

Commit 2d6c4d2b authored by Dmitriy Zaporozhets's avatar Dmitriy Zaporozhets
Browse files

Merge branch 'fix-huge-mr' into 'master'

Fix huge merge requests

* Refactored suppress logic for diffs
* Increase grit memory limit (should mix most of 502 errors when create MR)
* show first 100 files for huge diffs

Fixes #1424

See merge request !970
......@@ -112,7 +112,9 @@
.commit-stat-summary {
color: #666;
line-height: 2;
font-size: 14px;
font-weight: normal;
padding: 10px 0;
}
.commit-info-row {
......
......@@ -20,11 +20,10 @@ def show
end
begin
@suppress_diff = true if commit.diff_suppress? && !params[:force_show_diff]
@force_suppress_diff = commit.diff_force_suppress?
@diffs = @commit.diffs
rescue Grit::Git::GitTimeout
@suppress_diff = true
@status = :huge_commit
@diffs = []
@diff_timeout = true
end
@note = project.build_commit_note(commit)
......@@ -38,12 +37,7 @@ def show
}
respond_to do |format|
format.html do
if @status == :huge_commit
render "huge_commit" and return
end
end
format.html
format.diff { render text: @commit.to_diff }
format.patch { render text: @commit.to_patch }
end
......
......@@ -15,11 +15,7 @@ def show
@diffs = compare.diffs
@refs_are_same = compare.same
@line_notes = []
@timeout = compare.timeout
diff_line_count = Commit::diff_line_count(@diffs)
@suppress_diff = Commit::diff_suppress?(@diffs, diff_line_count) && !params[:force_show_diff]
@force_suppress_diff = Commit::diff_force_suppress?(@diffs, diff_line_count)
@diff_timeout = compare.timeout
end
def create
......
......@@ -34,6 +34,7 @@ def index
def show
@note_counts = Note.where(commit_id: @merge_request.commits.map(&:id)).
group(:commit_id).count
respond_to do |format|
format.html
format.diff { render text: @merge_request.to_diff(current_user) }
......@@ -43,16 +44,13 @@ def show
def diffs
@commit = @merge_request.last_commit
@comments_allowed = @reply_allowed = true
@comments_target = {noteable_type: 'MergeRequest',
noteable_id: @merge_request.id}
@comments_target = {
noteable_type: 'MergeRequest',
noteable_id: @merge_request.id
}
@line_notes = @merge_request.notes.where("line_code is not null")
diff_line_count = Commit::diff_line_count(@merge_request.diffs)
@suppress_diff = Commit::diff_suppress?(@merge_request.diffs, diff_line_count) && !params[:force_show_diff]
@force_suppress_diff = Commit::diff_force_suppress?(@merge_request.diffs, diff_line_count)
respond_to do |format|
format.html
format.json { render json: { html: view_to_html_string("projects/merge_requests/show/_diffs") } }
......@@ -60,54 +58,22 @@ def diffs
end
def new
params[:merge_request] ||= ActionController::Parameters.new(
source_project: @project
)
@merge_request = MergeRequest.new(merge_request_params)
@merge_request.source_project = @project unless @merge_request.source_project
@merge_request.target_project ||= (@project.forked_from_project || @project)
@target_branches = @merge_request.target_project.nil? ? [] : @merge_request.target_project.repository.branch_names
@merge_request.target_branch ||= @merge_request.target_project.default_branch
@source_project = @merge_request.source_project
if @merge_request.target_branch && @merge_request.source_branch
compare_action = Gitlab::Satellite::CompareAction.new(
current_user,
@merge_request.target_project,
@merge_request.target_branch,
@merge_request.source_project,
@merge_request.source_branch
)
@compare_failed = false
@commits = compare_action.commits
if @commits
@commits.map! { |commit| Commit.new(commit) }
@commit = @commits.first
else
# false value because failed to get commits from satellite
@commits = []
@compare_failed = true
end
@note_counts = Note.where(commit_id: @commits.map(&:id)).
group(:commit_id).count
begin
@diffs = compare_action.diffs
@merge_request.title = @merge_request.source_branch.titleize.humanize
@target_project = @merge_request.target_project
@target_repo = @target_project.repository
diff_line_count = Commit::diff_line_count(@diffs)
@suppress_diff = Commit::diff_suppress?(@diffs, diff_line_count)
@force_suppress_diff = @suppress_diff
rescue Gitlab::Satellite::BranchesWithoutParent
@error = "Selected branches have no common commit so they cannot be merged."
end
end
params[:merge_request] ||= ActionController::Parameters.new(source_project: @project)
@merge_request = MergeRequests::BuildService.new(project, current_user, merge_request_params).execute
@target_branches = if @merge_request.target_project
@merge_request.target_project.repository.branch_names
else
[]
end
@target_project = merge_request.target_project
@source_project = merge_request.source_project
@commits = @merge_request.compare_commits
@commit = @merge_request.compare_base_commit
@diffs = @merge_request.compare_diffs
@note_counts = Note.where(commit_id: @commits.map(&:id)).
group(:commit_id).count
end
def edit
......
module DiffHelper
def safe_diff_files(diffs)
if diff_hard_limit_enabled?
diffs.first(Commit::DIFF_HARD_LIMIT_FILES)
else
diffs.first(Commit::DIFF_SAFE_FILES)
end
end
def show_diff_size_warninig?(diffs)
safe_diff_files(diffs).size < diffs.size
end
def diff_hard_limit_enabled?
# Enabling hard limit allows user to see more diff information
if params[:force_show_diff].present?
true
else
false
end
end
end
......@@ -221,4 +221,10 @@ def project_last_activity(project)
"Never"
end
end
def contribution_guide_url(project)
if project && project.repository.contribution_guide
project_blob_path(project, tree_join(project.default_branch, project.repository.contribution_guide.name))
end
end
end
......@@ -12,6 +12,7 @@ class Commit
# User can force display of diff above this size
DIFF_SAFE_FILES = 100
DIFF_SAFE_LINES = 5000
# Commits above this size will not be rendered in HTML
DIFF_HARD_LIMIT_FILES = 1000
DIFF_HARD_LIMIT_LINES = 50000
......@@ -23,23 +24,7 @@ def decorate(commits)
# Calculate number of lines to render for diffs
def diff_line_count(diffs)
diffs.reduce(0){|sum, d| sum + d.diff.lines.count}
end
def diff_suppress?(diffs, line_count = nil)
# optimize - check file count first
return true if diffs.size > DIFF_SAFE_FILES
line_count ||= Commit::diff_line_count(diffs)
line_count > DIFF_SAFE_LINES
end
def diff_force_suppress?(diffs, line_count = nil)
# optimize - check file count first
return true if diffs.size > DIFF_HARD_LIMIT_FILES
line_count ||= Commit::diff_line_count(diffs)
line_count > DIFF_HARD_LIMIT_LINES
diffs.reduce(0) { |sum, d| sum + d.diff.lines.count }
end
end
......@@ -60,14 +45,6 @@ def diff_line_count
@diff_line_count
end
def diff_suppress?
Commit::diff_suppress?(self.diffs, diff_line_count)
end
def diff_force_suppress?
Commit::diff_force_suppress?(self.diffs, diff_line_count)
end
# Returns a string describing the commit for use in a link title
#
# Example
......
......@@ -42,6 +42,11 @@ class MergeRequest < ActiveRecord::Base
# It allows us to close or modify broken merge requests
attr_accessor :allow_broken
# Temporary fields to store compare vars
# when creating new merge request
attr_accessor :can_be_created, :compare_failed, :compare_base_commit,
:compare_commits, :compare_diffs
ActsAsTaggableOn.strict_case_match = true
acts_as_taggable_on :labels
......
module MergeRequests
class BuildService < MergeRequests::BaseService
def execute
merge_request = MergeRequest.new(params)
# Set MR attributes
merge_request.can_be_created = false
merge_request.compare_failed = false
merge_request.compare_commits = []
merge_request.compare_diffs = []
merge_request.source_project = project unless merge_request.source_project
merge_request.target_project ||= (project.forked_from_project || project)
merge_request.target_branch ||= merge_request.target_project.default_branch
unless merge_request.target_branch && merge_request.source_branch
return build_failed(merge_request, "You must select source and target branches")
end
# Generate suggested MR title based on source branch name
merge_request.title = merge_request.source_branch.titleize.humanize
# Try to compare branches to get commits list and diffs
compare_action = Gitlab::Satellite::CompareAction.new(
current_user,
merge_request.target_project,
merge_request.target_branch,
merge_request.source_project,
merge_request.source_branch
)
commits = compare_action.commits
# At this point we decide if merge request can be created
# If we have at least one commit to merge -> creation allowed
if commits.present?
merge_request.compare_commits = Commit.decorate(commits)
merge_request.compare_base_commit = Commit.new(commits.first)
merge_request.can_be_created = true
merge_request.compare_failed = false
# Try to collect diff for merge request.
diffs = compare_action.diffs
if diffs.present?
merge_request.compare_diffs = diffs
elsif diffs == false
# satellite timeout return false
merge_request.can_be_created = false
merge_request.compare_failed = true
end
else
merge_request.can_be_created = false
merge_request.compare_failed = true
end
merge_request
rescue Gitlab::Satellite::BranchesWithoutParent
return build_failed(merge_request, "Selected branches have no common commit so they cannot be merged.")
end
def build_failed(merge_request, message)
merge_request.errors.add(:base, message)
merge_request.compare_commits = []
merge_request.can_be_created = false
merge_request
end
end
end
= render "projects/commit/commit_box"
.alert.alert-danger
%h4 Commit diffs are too big to be displayed
= render "commit_box"
= render "projects/commits/diffs", diffs: @commit.diffs, project: @project
= render "projects/commits/diffs", diffs: @diffs, project: @project
= render "projects/notes/notes_with_form"
- file = project.repository.blob_at(@commit.id, diff.new_path)
- file = project.repository.blob_at(@commit.parent_id, diff.old_path) unless file
- return unless file
.diff-file{id: "diff-#{i}"}
.diff-header{id: "file-path-#{hexdigest(diff.new_path || diff.old_path)}"}
- if diff.deleted_file
%span= diff.old_path
.diff-btn-group
- if @commit.parent_ids.present?
= link_to project_blob_path(project, tree_join(@commit.parent_id, diff.new_path)), { class: 'btn btn-small view-file' } do
View file @
%span.commit-short-id= @commit.short_id(6)
- else
%span= diff.new_path
- if diff_file_mode_changed?(diff)
%span.file-mode= "#{diff.a_mode}#{diff.b_mode}"
.diff-btn-group
= link_to "#", class: "js-toggle-diff-comments btn btn-small" do
%i.icon-chevron-down
Diff comments
&nbsp;
- if @merge_request && @merge_request.source_project
= link_to project_edit_tree_path(@merge_request.source_project, tree_join(@merge_request.source_branch, diff.new_path), from_merge_request_id: @merge_request.id), { class: 'btn btn-small' } do
Edit
&nbsp;
= link_to project_blob_path(project, tree_join(@commit.id, diff.new_path)), { class: 'btn btn-small view-file' } do
View file @
%span.commit-short-id= @commit.short_id(6)
.diff-content
-# Skipp all non non-supported blobs
- return unless file.respond_to?('text?')
- if file.text?
- if params[:view] == 'parallel'
= render "projects/commits/parallel_view", diff: diff, project: project, file: file, index: i
- else
= render "projects/commits/text_file", diff: diff, index: i
- elsif file.image?
- old_file = project.repository.blob_at(@commit.parent_id, diff.old_path) if @commit.parent_id
= render "projects/commits/image", diff: diff, old_file: old_file, file: file, index: i
- else
.nothing-here-block No preview for this file type
%ul.bordered-list
- diffs.each_with_index do |diff, i|
%li
- if diff.deleted_file
%span.deleted-file
%a{href: "#diff-#{i}"}
%i.icon-minus
= diff.old_path
- elsif diff.renamed_file
%span.renamed-file
%a{href: "#diff-#{i}"}
%i.icon-minus
= diff.old_path
= "->"
= diff.new_path
- elsif diff.new_file
%span.new-file
%a{href: "#diff-#{i}"}
%i.icon-plus
= diff.new_path
- else
%span.edit-file
%a{href: "#diff-#{i}"}
%i.icon-adjust
= diff.new_path
.js-toggle-container
.commit-stat-summary
Showing
%strong.cdark #{pluralize(diffs.count, "changed file")}
- if current_controller?(:commit)
- unless @commit.has_zero_stats?
with
%strong.cgreen #{@commit.stats.additions} additions
and
%strong.cred #{@commit.stats.deletions} deletions
&nbsp;
= link_to '#', class: 'btn btn-small js-toggle-button' do
Show diff stats
%i.icon-chevron-down
.file-stats.js-toggle-content.hide
%ul.bordered-list
- diffs.each_with_index do |diff, i|
%li
- if diff.deleted_file
%span.deleted-file
%a{href: "#diff-#{i}"}
%i.icon-minus
= diff.old_path
- elsif diff.renamed_file
%span.renamed-file
%a{href: "#diff-#{i}"}
%i.icon-minus
= diff.old_path
= "->"
= diff.new_path
- elsif diff.new_file
%span.new-file
%a{href: "#diff-#{i}"}
%i.icon-plus
= diff.new_path
- else
%span.edit-file
%a{href: "#diff-#{i}"}
%i.icon-adjust
= diff.new_path
.bs-callout.bs-callout-warning
%h4
Too many changes.
.pull-right
- unless diff_hard_limit_enabled?
= link_to "Reload with full diff", url_for(params.merge(force_show_diff: true)), class: "btn btn-small btn-warning"
- if current_controller?(:commit) or current_controller?(:merge_requests)
- if current_controller?(:commit)
= link_to "Plain diff", project_commit_path(@project, @commit, format: :diff), class: "btn btn-warning btn-small"
= link_to "Email patch", project_commit_path(@project, @commit, format: :patch), class: "btn btn-warning btn-small"
- elsif @merge_request && @merge_request.persisted?
= link_to "Plain diff", project_merge_request_path(@project, @merge_request, format: :diff), class: "btn btn-warning btn-small"
= link_to "Email patch", project_merge_request_path(@project, @merge_request, format: :patch), class: "btn btn-warning btn-small"
%p
To preserve performance only
%strong #{safe_diff_files(diffs).size} of #{diffs.size}
files displayed.
- @suppress_diff ||= @suppress_diff || @force_suppress_diff
- if @suppress_diff
.alert.alert-warning
%p
%strong Warning! This is a large diff.
%p
To preserve performance the diff is not shown.
- if current_controller?(:commit) or current_controller?(:merge_requests)
- if current_controller?(:commit)
Please, download the diff as
= link_to "plain diff", project_commit_path(@project, @commit, format: :diff), class: "underlined-link"
or
= link_to "email patch", project_commit_path(@project, @commit, format: :patch), class: "underlined-link"
instead.
- elsif @merge_request && @merge_request.persisted?
Please, download the diff as
= link_to "plain diff", project_merge_request_path(@project, @merge_request, format: :diff), class: "underlined-link"
or
= link_to "email patch", project_merge_request_path(@project, @merge_request, format: :patch), class: "underlined-link"
instead.
- unless @force_suppress_diff
%p
If you still want to see the diff
= link_to "click this link", url_for(force_show_diff: true), class: "underlined-link"
%p.commit-stat-summary
Showing
%strong.cdark #{pluralize(diffs.count, "changed file")}
- if current_controller?(:commit)
- unless @commit.has_zero_stats?
with
%strong.cgreen #{@commit.stats.additions} additions
and
%strong.cred #{@commit.stats.deletions} deletions
- if params[:view] == 'parallel'
= link_to "Inline Diff", url_for(view: 'inline'), {id: "commit-diff-viewtype", class: 'btn btn-tiny pull-right'}
- else
= link_to "Side-by-side Diff", url_for(view: 'parallel'), {id: "commit-diff-viewtype", class: 'btn btn-tiny pull-right'}
.file-stats
= render "projects/commits/diff_head", diffs: diffs
.row
.col-md-8
= render 'projects/commits/diff_stats', diffs: diffs
.col-md-4
%ul.nav.nav-tabs
%li.pull-right{class: params[:view] == 'parallel' ? 'active' : ''}
= link_to "Side-by-side Diff", url_for(view: 'parallel'), {id: "commit-diff-viewtype"}
%li.pull-right{class: params[:view] != 'parallel' ? 'active' : ''}
= link_to "Inline Diff", url_for(view: 'inline'), {id: "commit-diff-viewtype"}
- if show_diff_size_warninig?(diffs)
= render 'projects/commits/diff_warning', diffs: diffs
.files
- unless @suppress_diff
- diffs.each_with_index do |diff, i|
- file = project.repository.blob_at(@commit.id, diff.new_path)
- file = project.repository.blob_at(@commit.parent_id, diff.old_path) unless file
- next unless file
.diff-file{id: "diff-#{i}"}
.diff-header{id: "file-path-#{hexdigest(diff.new_path || diff.old_path)}"}
- if diff.deleted_file
%span= diff.old_path
.diff-btn-group
- if @commit.parent_ids.present?
= link_to project_blob_path(project, tree_join(@commit.parent_id, diff.new_path)), { class: 'btn btn-small view-file' } do
View file @
%span.commit-short-id= @commit.short_id(6)
- else
%span= diff.new_path
- if diff_file_mode_changed?(diff)
%span.file-mode= "#{diff.a_mode}#{diff.b_mode}"
.diff-btn-group
= link_to "#", class: "js-toggle-diff-comments btn btn-small" do
%i.icon-chevron-down
Diff comments
&nbsp;
- safe_diff_files(diffs).each_with_index do |diff, i|
= render 'projects/commits/diff_file', diff: diff, i: i, project: project
- if @merge_request && @merge_request.source_project
= link_to project_edit_tree_path(@merge_request.source_project, tree_join(@merge_request.source_branch, diff.new_path), from_merge_request_id: @merge_request.id), { class: 'btn btn-small' } do
Edit
&nbsp;
= link_to project_blob_path(project, tree_join(@commit.id, diff.new_path)), { class: 'btn btn-small view-file' } do
View file @
%span.commit-short-id= @commit.short_id(6)
.diff-content
-# Skipp all non non-supported blobs
- next unless file.respond_to?('text?')
- if file.text?
- if params[:view] == 'parallel'
= render "projects/commits/parallel_view", diff: diff, project: project, file: file, index: i
- else
= render "projects/commits/text_file", diff: diff, index: i
- elsif file.image?
- old_file = project.repository.blob_at(@commit.parent_id, diff.old_path) if @commit.parent_id
= render "projects/commits/image", diff: diff, old_file: old_file, file: file, index: i
- else
.nothing-here-block No preview for this file type
- if @diff_timeout
.alert.alert-danger
%h4
Failed to collect changes
%p
Maybe diff is really big and operation failed with timeout. Try to get diff localy
- too_big = diff.diff.lines.count > 1000
- too_big = diff.diff.lines.count > Commit::DIFF_SAFE_LINES
- if too_big
%a.supp_diff_link Changes suppressed. Click to show
......
......@@ -18,18 +18,7 @@
- else
%ul.well-list= render Commit.decorate(@commits), project: @project