Improved large commit handling.

Previously, only number of changed files mattered. Now, number of lines to render in the diff are also taken into account.

A hard limit is set, above which diffs are not rendered and users are not allowed to override that. This prevents high server
resource usage with huge commits.

Related to #1745, #2259

In addition, handle large commits for MergeRequests and Compare controllers.

Also fixes a bug where diffs are loaded twice, if user goes directly to merge_requests/:id/diffs URL.

7 files changed, 71 insertions, 8 deletions

diff --git a/app/assets/javascripts/merge_requests.js.coffee b/app/assets/javascripts/merge_requests.js.coffee
index 153198ca5c5..5400bc5c1ad 100644
--- a/app/assets/javascripts/merge_requests.js.coffee
+++ b/app/assets/javascripts/merge_requests.js.coffee
@@ -11,7 +11,7 @@ class MergeRequest
 
   constructor: (@opts) ->
     this.$el = $('.merge-request')
-    @diffs_loaded = false
+    @diffs_loaded = if @opts.action == 'diffs' then true else false
     @commits_loaded = false
 
     this.activateTab(@opts.action)
diff --git a/app/contexts/commit_load_context.rb b/app/contexts/commit_load_context.rb
index 2cf5420d62d..2930c5b1668 100644
--- a/app/contexts/commit_load_context.rb
+++ b/app/contexts/commit_load_context.rb
@@ -20,7 +20,8 @@ class CommitLoadContext < BaseContext
       result[:notes_count] = project.notes.for_commit_id(commit.id).count
 
       begin
-        result[:suppress_diff] = true if commit.diffs.size > Commit::DIFF_SAFE_SIZE && !params[:force_show_diff]
+        result[:suppress_diff] = true if commit.diff_suppress? && !params[:force_show_diff]
+        result[:force_suppress_diff] = commit.diff_force_suppress?
       rescue Grit::Git::GitTimeout
         result[:suppress_diff] = true
         result[:status] = :huge_commit
diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb
index 6a2d2315c1d..bdc501d73bb 100644
--- a/app/controllers/projects/commit_controller.rb
+++ b/app/controllers/projects/commit_controller.rb
@@ -18,6 +18,7 @@ class Projects::CommitController < Projects::ApplicationController
     end
 
     @suppress_diff = result[:suppress_diff]
+    @force_suppress_diff = result[:force_suppress_diff]
 
     @note        = result[:note]
     @line_notes  = result[:line_notes]
diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb
index 126a2ea50c9..d7e660dac22 100644
--- a/app/controllers/projects/compare_controller.rb
+++ b/app/controllers/projects/compare_controller.rb
@@ -15,6 +15,10 @@ class Projects::CompareController < Projects::ApplicationController
     @diffs         = compare.diffs
     @refs_are_same = compare.same
     @line_notes    = []
+
+    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)
   end
 
   def create
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 55d2c3f04fc..3bc50b0418f 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -40,6 +40,10 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     @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)
   end
 
   def new
diff --git a/app/models/commit.rb b/app/models/commit.rb
index f8ca6a5fe82..dd1f9801878 100644
--- a/app/models/commit.rb
+++ b/app/models/commit.rb
@@ -6,15 +6,41 @@ class Commit
 
   attr_mentionable :safe_message
 
-  # Safe amount of files with diffs in one commit to render
+  # Safe amount of changes (files and lines) in one commit to render
   # Used to prevent 500 error on huge commits by suppressing diff
   #
-  DIFF_SAFE_SIZE = 100
+  # 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 = 500
+  DIFF_HARD_LIMIT_LINES = 10000
 
   def self.decorate(commits)
     commits.map { |c| self.new(c) }
   end
 
+  # Calculate number of lines to render for diffs
+  def self.diff_line_count(diffs)
+    diffs.reduce(0){|sum, d| sum + d.diff.lines.count}
+  end
+
+  def self.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 self.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
+  end
+
   attr_accessor :raw
 
   def initialize(raw_commit)
@@ -27,6 +53,19 @@ class Commit
     @raw.id
   end
 
+  def diff_line_count
+    @diff_line_count ||= Commit::diff_line_count(self.diffs)
+    @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/views/projects/commits/_diffs.html.haml b/app/views/projects/commits/_diffs.html.haml
index 5a3a9bd16af..c51f1b6eff5 100644
--- a/app/views/projects/commits/_diffs.html.haml
+++ b/app/views/projects/commits/_diffs.html.haml
@@ -1,11 +1,25 @@
+- @suppress_diff ||= @suppress_diff || @force_suppress_diff
 - if @suppress_diff
   .alert.alert-block
     %p
-      %strong Warning! Large commit with more than #{Commit::DIFF_SAFE_SIZE} files changed.
-    %p To preserve performance the diff is not shown.
+      %strong Warning! This is a large diff.
     %p
-      But if you still want to see the diff
-      = link_to "click this link", project_commit_path(project, @commit, force_show_diff: true), class: "underlined_link"
+      To preserve performance the diff is not shown.
+      - if current_controller?(:commit) or current_controller?(:merge_requests)
+        Please, download the diff as
+        - if current_controller?(:commit)
+          = 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"
+        - else
+          = 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