From be5b6db883ee634dc53e0a50ac57f424507d7a7d Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 15 Jul 2014 18:28:21 +0300 Subject: [PATCH] Refactor diff suppress logic and diff views Signed-off-by: Dmitriy Zaporozhets --- app/controllers/projects/commit_controller.rb | 14 +-- .../projects/compare_controller.rb | 6 +- .../projects/merge_requests_controller.rb | 18 +--- app/helpers/diff_helper.rb | 22 ++++ app/models/commit.rb | 27 +---- app/models/merge_request.rb | 3 +- app/services/merge_requests/build_service.rb | 9 +- .../projects/commit/huge_commit.html.haml | 3 - app/views/projects/commit/show.html.haml | 2 +- .../projects/commits/_diff_file.html.haml | 48 +++++++++ .../projects/commits/_diff_head.html.haml | 26 ----- .../projects/commits/_diff_stats.html.haml | 41 +++++++ .../projects/commits/_diff_warning.html.haml | 19 ++++ app/views/projects/commits/_diffs.html.haml | 102 +++--------------- .../projects/commits/_text_file.html.haml | 2 +- app/views/projects/compare/show.html.haml | 13 +-- .../merge_requests/_new_compare.html.haml | 2 +- 17 files changed, 165 insertions(+), 192 deletions(-) create mode 100644 app/helpers/diff_helper.rb delete mode 100644 app/views/projects/commit/huge_commit.html.haml create mode 100644 app/views/projects/commits/_diff_file.html.haml delete mode 100644 app/views/projects/commits/_diff_head.html.haml create mode 100644 app/views/projects/commits/_diff_stats.html.haml create mode 100644 app/views/projects/commits/_diff_warning.html.haml diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb index 860ab408299..c344297ba8a 100644 --- a/app/controllers/projects/commit_controller.rb +++ b/app/controllers/projects/commit_controller.rb @@ -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 diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb index 234b6058ff0..eae96396574 100644 --- a/app/controllers/projects/compare_controller.rb +++ b/app/controllers/projects/compare_controller.rb @@ -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 diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 7f9c238507e..fcc6384e27c 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -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 diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb new file mode 100644 index 00000000000..ee4d4fbdff5 --- /dev/null +++ b/app/helpers/diff_helper.rb @@ -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 diff --git a/app/models/commit.rb b/app/models/commit.rb index 82876e10446..ff5392957ce 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -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 diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 530e1add8e7..28486fb41c6 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -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 diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb index 9c28d3d82d8..fb487539687 100644 --- a/app/services/merge_requests/build_service.rb +++ b/app/services/merge_requests/build_service.rb @@ -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) diff --git a/app/views/projects/commit/huge_commit.html.haml b/app/views/projects/commit/huge_commit.html.haml deleted file mode 100644 index 398ce771426..00000000000 --- a/app/views/projects/commit/huge_commit.html.haml +++ /dev/null @@ -1,3 +0,0 @@ -= render "projects/commit/commit_box" -.alert.alert-danger - %h4 Commit diffs are too big to be displayed diff --git a/app/views/projects/commit/show.html.haml b/app/views/projects/commit/show.html.haml index da1b4c10f87..0a15aef6cb7 100644 --- a/app/views/projects/commit/show.html.haml +++ b/app/views/projects/commit/show.html.haml @@ -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" diff --git a/app/views/projects/commits/_diff_file.html.haml b/app/views/projects/commits/_diff_file.html.haml new file mode 100644 index 00000000000..45d1cd9c9a0 --- /dev/null +++ b/app/views/projects/commits/_diff_file.html.haml @@ -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 + diff --git a/app/views/projects/commits/_diff_head.html.haml b/app/views/projects/commits/_diff_head.html.haml deleted file mode 100644 index 5aa542287fe..00000000000 --- a/app/views/projects/commits/_diff_head.html.haml +++ /dev/null @@ -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 - diff --git a/app/views/projects/commits/_diff_stats.html.haml b/app/views/projects/commits/_diff_stats.html.haml new file mode 100644 index 00000000000..846a1ee10e6 --- /dev/null +++ b/app/views/projects/commits/_diff_stats.html.haml @@ -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 + diff --git a/app/views/projects/commits/_diff_warning.html.haml b/app/views/projects/commits/_diff_warning.html.haml new file mode 100644 index 00000000000..05d516efa11 --- /dev/null +++ b/app/views/projects/commits/_diff_warning.html.haml @@ -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. + diff --git a/app/views/projects/commits/_diffs.html.haml b/app/views/projects/commits/_diffs.html.haml index e062e9b8d54..64d6a2f09cf 100644 --- a/app/views/projects/commits/_diffs.html.haml +++ b/app/views/projects/commits/_diffs.html.haml @@ -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 diff --git a/app/views/projects/commits/_text_file.html.haml b/app/views/projects/commits/_text_file.html.haml index 8ced4133294..f5b0d711416 100644 --- a/app/views/projects/commits/_text_file.html.haml +++ b/app/views/projects/commits/_text_file.html.haml @@ -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 diff --git a/app/views/projects/compare/show.html.haml b/app/views/projects/compare/show.html.haml index b232d2a6b26..240bfe7484e 100644 --- a/app/views/projects/compare/show.html.haml +++ b/app/views/projects/compare/show.html.haml @@ -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 diff --git a/app/views/projects/merge_requests/_new_compare.html.haml b/app/views/projects/merge_requests/_new_compare.html.haml index 76b5db419f7..99726172154 100644 --- a/app/views/projects/merge_requests/_new_compare.html.haml +++ b/app/views/projects/merge_requests/_new_compare.html.haml @@ -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.