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.

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)

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

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]

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

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

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

app/views/projects/commits/_diffs.html.haml

+- @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

features/project/commits/commits.feature

@@ -27,3 +27,11 @@ Feature: Project Browse commits
   Scenario: I browse commits stats
     Given I visit my project's commits stats page
     Then I see commits stats
+
+  Scenario: I browse big commit
+    Given I visit big commit page
+    Then I see big commit warning
+
+  Scenario: I browse huge commit
+    Given I visit huge commit page
+    Then I see huge commit message

features/steps/project/project_browse_commits.rb

@@ -58,4 +58,24 @@ class ProjectBrowseCommits < Spinach::FeatureSteps
     page.should have_content 'Total commits'
     page.should have_content 'Authors'
   end
+
+  Given 'I visit big commit page' do
+    visit project_commit_path(@project, BigCommits::BIG_COMMIT_ID)
+  end
+
+  Then 'I see big commit warning' do
+    page.should have_content BigCommits::BIG_COMMIT_MESSAGE
+    page.should have_content "Warning! This is a large diff"
+    page.should have_content "If you still want to see the diff"
+  end
+
+  Given 'I visit huge commit page' do
+    visit project_commit_path(@project, BigCommits::HUGE_COMMIT_ID)
+  end
+
+  Then 'I see huge commit message' do
+    page.should have_content BigCommits::HUGE_COMMIT_MESSAGE
+    page.should have_content "Warning! This is a large diff"
+    page.should_not have_content "If you still want to see the diff"
+  end
 end

features/support/env.rb

@@ -14,7 +14,7 @@ require 'spinach/capybara'
 require 'sidekiq/testing/inline'
-%w(valid_commit select2_helper chosen_helper test_env).each do |f|
+%w(valid_commit big_commits select2_helper chosen_helper test_env).each do |f|
   require Rails.root.join('spec', 'support', f)
 end

spec/support/big_commits.rb

+module BigCommits
+  HUGE_COMMIT_ID = "7f92534f767fa20357a11c63f973ae3b79cc5b85"
+  HUGE_COMMIT_MESSAGE = "pybments.rb version up. gitignore improved"
+
+  BIG_COMMIT_ID = "d62200cad430565bd9f80befaf329297120330b5"
+  BIG_COMMIT_MESSAGE = "clean-up code"
+end
+