Commit c86553cd836b7be3948ace41ef47f85776a48a97
Exists in
spb-stable
and in
3 other branches
Merge branch 'refactor_compare' into 'master'
Refactor Compare logic Updated gitlab_git to version 5.4.0. Fixes MR diff download link in case of huge diff. Fixes #1026
Showing
9 changed files
with
27 additions
and
20 deletions
Show diff stats
Gemfile
| ... | ... | @@ -29,7 +29,7 @@ gem 'omniauth-github' |
| 29 | 29 | |
| 30 | 30 | # Extracting information from a git repository |
| 31 | 31 | # Provide access to Gitlab::Git library |
| 32 | -gem "gitlab_git", '~> 5.3.0' | |
| 32 | +gem "gitlab_git", '~> 5.4.0' | |
| 33 | 33 | |
| 34 | 34 | # Ruby/Rack Git Smart-HTTP Server Handler |
| 35 | 35 | gem 'gitlab-grack', '~> 2.0.0.pre', require: 'grack' | ... | ... |
Gemfile.lock
| ... | ... | @@ -177,7 +177,7 @@ GEM |
| 177 | 177 | charlock_holmes (~> 0.6.6) |
| 178 | 178 | escape_utils (~> 0.2.4) |
| 179 | 179 | mime-types (~> 1.19) |
| 180 | - gitlab_git (5.3.0) | |
| 180 | + gitlab_git (5.4.0) | |
| 181 | 181 | activesupport (~> 4.0.0) |
| 182 | 182 | charlock_holmes (~> 0.6.9) |
| 183 | 183 | gitlab-grit (~> 2.6.1) |
| ... | ... | @@ -579,7 +579,7 @@ DEPENDENCIES |
| 579 | 579 | gitlab-gollum-lib (~> 1.1.0) |
| 580 | 580 | gitlab-grack (~> 2.0.0.pre) |
| 581 | 581 | gitlab-linguist (~> 3.0.0) |
| 582 | - gitlab_git (~> 5.3.0) | |
| 582 | + gitlab_git (~> 5.4.0) | |
| 583 | 583 | gitlab_meta (= 6.0) |
| 584 | 584 | gitlab_omniauth-ldap (= 1.0.4) |
| 585 | 585 | gon (~> 5.0.0) | ... | ... |
app/controllers/projects/compare_controller.rb
| ... | ... | @@ -15,11 +15,7 @@ class Projects::CompareController < Projects::ApplicationController |
| 15 | 15 | @diffs = compare.diffs |
| 16 | 16 | @refs_are_same = compare.same |
| 17 | 17 | @line_notes = [] |
| 18 | - | |
| 19 | - if @diffs == [Gitlab::Git::Diff::BROKEN_DIFF] | |
| 20 | - @diffs = [] | |
| 21 | - @timeout = true | |
| 22 | - end | |
| 18 | + @timeout = compare.timeout | |
| 23 | 19 | |
| 24 | 20 | diff_line_count = Commit::diff_line_count(@diffs) |
| 25 | 21 | @suppress_diff = Commit::diff_suppress?(@diffs, diff_line_count) && !params[:force_show_diff] | ... | ... |
app/mailers/emails/projects.rb
| ... | ... | @@ -17,6 +17,7 @@ module Emails |
| 17 | 17 | def repository_push_email(project_id, recipient, author_id, branch, compare) |
| 18 | 18 | @project = Project.find(project_id) |
| 19 | 19 | @author = User.find(author_id) |
| 20 | + @compare = compare | |
| 20 | 21 | @commits = Commit.decorate(compare.commits) |
| 21 | 22 | @diffs = compare.diffs |
| 22 | 23 | @branch = branch | ... | ... |
app/models/merge_request_diff.rb
| ... | ... | @@ -148,13 +148,11 @@ class MergeRequestDiff < ActiveRecord::Base |
| 148 | 148 | Gitlab::Git::Diff.between(repository, source_branch, target_branch) |
| 149 | 149 | end |
| 150 | 150 | |
| 151 | - if diffs == broken_diffs | |
| 152 | - self.state = :timeout | |
| 153 | - diffs = [] | |
| 154 | - end | |
| 155 | - | |
| 156 | 151 | diffs ||= [] |
| 157 | 152 | diffs |
| 153 | + rescue Gitlab::Git::Diff::TimeoutError => ex | |
| 154 | + self.state = :timeout | |
| 155 | + diffs = [] | |
| 158 | 156 | end |
| 159 | 157 | |
| 160 | 158 | def repository | ... | ... |
app/views/notify/repository_push_email.html.haml
app/views/notify/repository_push_email.text.haml
| ... | ... | @@ -18,3 +18,8 @@ Diff: |
| 18 | 18 | = diff.new_path || diff.old_path |
| 19 | 19 | \===================================== |
| 20 | 20 | = diff.diff |
| 21 | +\ | |
| 22 | +- if @compare.timeout | |
| 23 | + Huge diff. To prevent performance issues it was hidden | |
| 24 | +- elsif @compare.commits_over_limit? | |
| 25 | + Diff for big amount of commits is disabled | ... | ... |
app/views/projects/merge_requests/show/_diffs.html.haml
| ... | ... | @@ -3,8 +3,10 @@ |
| 3 | 3 | - elsif @merge_request_diff.empty? |
| 4 | 4 | %h4.nothing_here_message Nothing to merge from #{@merge_request.source_branch} into #{@merge_request.target_branch} |
| 5 | 5 | - else |
| 6 | - %h4.nothing_here_message | |
| 7 | - Can't load diff. | |
| 8 | - You can | |
| 9 | - = link_to "download it", project_merge_request_path(@merge_request.source_project, @merge_request), format: :diff, class: "vlink" | |
| 10 | - instead. | |
| 6 | + .bs-callout.bs-callout-warning | |
| 7 | + %h4 | |
| 8 | + Diff for this comparison is extremely large. | |
| 9 | + %p | |
| 10 | + You can | |
| 11 | + = link_to "download it", project_merge_request_path(@merge_request.source_project, @merge_request, format: :diff), class: "vlink" | |
| 12 | + instead. | ... | ... |
app/workers/emails_on_push_worker.rb
| ... | ... | @@ -13,13 +13,13 @@ class EmailsOnPushWorker |
| 13 | 13 | return true |
| 14 | 14 | end |
| 15 | 15 | |
| 16 | - compare = Gitlab::Git::Compare.new(project.repository.raw_repository, before_sha, after_sha) | |
| 16 | + compare = Gitlab::Git::Compare.new(project.repository.raw_repository, before_sha, after_sha, MergeRequestDiff::COMMITS_SAFE_SIZE) | |
| 17 | 17 | |
| 18 | 18 | # Do not send emails if git compare failed |
| 19 | 19 | return false unless compare && compare.commits.present? |
| 20 | 20 | |
| 21 | 21 | recipients.split(" ").each do |recipient| |
| 22 | - Notify.delay.repository_push_email(project_id, recipient, author_id, branch, compare) | |
| 22 | + Notify.repository_push_email(project_id, recipient, author_id, branch, compare).deliver | |
| 23 | 23 | end |
| 24 | 24 | end |
| 25 | 25 | end | ... | ... |