Refactor diff suppress logic and diff views
Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
This commit is contained in:
parent
3dc347a369
commit
be5b6db883
|
@ -20,11 +20,10 @@ class Projects::CommitController < Projects::ApplicationController
|
|||
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 @@ class Projects::CommitController < Projects::ApplicationController
|
|||
}
|
||||
|
||||
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 @@ class Projects::CompareController < Projects::ApplicationController
|
|||
@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 @@ class Projects::MergeRequestsController < Projects::ApplicationController
|
|||
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 @@ class Projects::MergeRequestsController < Projects::ApplicationController
|
|||
|
||||
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") } }
|
||||
|
@ -76,12 +74,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController
|
|||
@diffs = @merge_request.compare_diffs
|
||||
@note_counts = Note.where(commit_id: @commits.map(&:id)).
|
||||
group(:commit_id).count
|
||||
|
||||
if @diffs.any?
|
||||
@diff_line_count = Commit::diff_line_count(@diffs)
|
||||
@suppress_diff = Commit::diff_suppress?(@diffs, @diff_line_count)
|
||||
@force_suppress_diff = @suppress_diff
|
||||
end
|
||||
end
|
||||
|
||||
def edit
|
||||
|
|
|
@ -0,0 +1,22 @@
|
|||
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
|
|
@ -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 @@ class Commit
|
|||
|
||||
# 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 @@ class Commit
|
|||
@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
|
||||
|
|
|
@ -44,7 +44,8 @@ class MergeRequest < ActiveRecord::Base
|
|||
|
||||
# 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
|
||||
attr_accessor :can_be_created, :compare_failed, :compare_base_commit,
|
||||
:compare_commits, :compare_diffs
|
||||
|
||||
ActsAsTaggableOn.strict_case_match = true
|
||||
acts_as_taggable_on :labels
|
||||
|
|
|
@ -39,12 +39,15 @@ module MergeRequests
|
|||
merge_request.compare_failed = false
|
||||
|
||||
# Try to collect diff for merge request.
|
||||
# Note: even if diff is huge and we can't show it - we still should allow
|
||||
# people to create MR.
|
||||
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
|
||||
|
@ -55,8 +58,6 @@ module MergeRequests
|
|||
|
||||
rescue Gitlab::Satellite::BranchesWithoutParent
|
||||
return build_failed(merge_request, "Selected branches have no common commit so they cannot be merged.")
|
||||
#rescue
|
||||
#return build_failed(merge_request, "We cannot create merge request because of huge diff.")
|
||||
end
|
||||
|
||||
def build_failed(merge_request, message)
|
||||
|
|
|
@ -1,3 +0,0 @@
|
|||
= render "projects/commit/commit_box"
|
||||
.alert.alert-danger
|
||||
%h4 Commit diffs are too big to be displayed
|
|
@ -1,3 +1,3 @@
|
|||
= 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"
|
||||
|
|
|
@ -0,0 +1,48 @@
|
|||
- 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
|
||||
|
||||
|
||||
- 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
|
||||
|
||||
|
||||
= 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
|
||||
|
|
@ -1,26 +0,0 @@
|
|||
%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
|
||||
|
|
@ -0,0 +1,41 @@
|
|||
.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
|
||||
|
||||
= 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
|
||||
|
|
@ -0,0 +1,19 @@
|
|||
.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.
|
||||
|
|
@ -1,47 +1,6 @@
|
|||
- @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"
|
||||
|
||||
|
||||
.row
|
||||
.col-md-8
|
||||
.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
|
||||
|
||||
= link_to '#', class: 'btn btn-small js-toggle-button' do
|
||||
Show diff stats
|
||||
%i.icon-chevron-down
|
||||
.file-stats.js-toggle-content.hide
|
||||
= render "projects/commits/diff_head", diffs: diffs
|
||||
= render 'projects/commits/diff_stats', diffs: diffs
|
||||
.col-md-4
|
||||
%ul.nav.nav-tabs
|
||||
%li.pull-right{class: params[:view] == 'parallel' ? 'active' : ''}
|
||||
|
@ -49,53 +8,16 @@
|
|||
%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
|
||||
- safe_diff_files(diffs).each_with_index do |diff, i|
|
||||
= render 'projects/commits/diff_file', diff: diff, i: i, project: project
|
||||
|
||||
.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
|
||||
|
||||
|
||||
- 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
|
||||
|
||||
|
||||
= 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
|
||||
|
|
|
@ -1,4 +1,4 @@
|
|||
- 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
|
||||
|
||||
%h4 Changes
|
||||
- if @diffs.present?
|
||||
= render "projects/commits/diffs", diffs: @diffs, project: @project
|
||||
- elsif @commits.size > MergeRequestDiff::COMMITS_SAFE_SIZE
|
||||
.bs-callout.bs-callout-danger
|
||||
%h4 This comparison includes more than #{MergeRequestDiff::COMMITS_SAFE_SIZE} commits.
|
||||
%p To preserve performance the line changes are not shown.
|
||||
- elsif @timeout
|
||||
.bs-callout.bs-callout-danger
|
||||
%h4 Number of changed files for this comparison is extremely large.
|
||||
%p Use command line to browse through changes for this comparison.
|
||||
|
||||
= render "projects/commits/diffs", diffs: @diffs, project: @project
|
||||
|
||||
- else
|
||||
.light-well
|
||||
|
|
|
@ -33,7 +33,7 @@
|
|||
%div= msg
|
||||
|
||||
- elsif @merge_request.source_branch.present? && @merge_request.target_branch.present?
|
||||
- if @compare_failed
|
||||
- if @merge_request.compare_failed
|
||||
.alert.alert-danger
|
||||
%h4 Compare failed
|
||||
%p We can't compare selected branches. It may be because of huge diff or satellite timeout. Please try again or select different branches.
|
||||
|
|
Loading…
Reference in New Issue