Merge pull request #5014 from bladealslayer/feature/big-commit-improvements

Improved large commit handling.

Merge pull request #5014 from bladealslayer/feature/big-commit-improvements
Improved large commit handling.
Dmitriy Zaporozhets
2 parents 71d31a38 01ff084a
Showing 11 changed files with 108 additions and 9 deletions Show diff stats
app/assets/javascripts/merge_requests.js.coffee
app/contexts/commit_load_context.rb
app/controllers/projects/commit_controller.rb
app/controllers/projects/compare_controller.rb
app/controllers/projects/merge_requests_controller.rb
app/models/commit.rb
app/views/projects/commits/_diffs.html.haml
features/project/commits/commits.feature
features/steps/project/project_browse_commits.rb
features/support/env.rb
spec/support/big_commits.rb
@@ -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)
@@ -20,7 +20,8 @@ class CommitLoadContext &lt; 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
@@ -18,6 +18,7 @@ class Projects::CommitController &lt; Projects::ApplicationController
     end
     @suppress_diff = result[:suppress_diff]
+    @force_suppress_diff = result[:force_suppress_diff]
     @note        = result[:note]
     @line_notes  = result[:line_notes]
@@ -15,6 +15,10 @@ class Projects::CompareController &lt; 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
@@ -40,6 +40,10 @@ class Projects::MergeRequestsController &lt; 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
@@ -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
+- @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
@@ -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
@@ -58,4 +58,24 @@ class ProjectBrowseCommits &lt; 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
@@ -14,7 +14,7 @@ require &#39;spinach/capybara&#39;
 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
@@ -0,0 +1,8 @@
+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
+
	@@ -11,7 +11,7 @@ class MergeRequest		@@ -11,7 +11,7 @@ class MergeRequest
11		11
12	constructor: (@opts) ->	12	constructor: (@opts) ->
13	this.$el = $('.merge-request')	13	this.$el = $('.merge-request')
14	- @diffs_loaded = false	14	+ @diffs_loaded = if @opts.action == 'diffs' then true else false
15	@commits_loaded = false	15	@commits_loaded = false
16		16
17	this.activateTab(@opts.action)	17	this.activateTab(@opts.action)
	@@ -18,6 +18,7 @@ class Projects::CommitController < Projects::ApplicationController		@@ -18,6 +18,7 @@ class Projects::CommitController < Projects::ApplicationController
18	end	18	end
19		19
20	@suppress_diff = result[:suppress_diff]	20	@suppress_diff = result[:suppress_diff]
		21	+ @force_suppress_diff = result[:force_suppress_diff]
21		22
22	@note = result[:note]	23	@note = result[:note]
23	@line_notes = result[:line_notes]	24	@line_notes = result[:line_notes]
	@@ -6,15 +6,41 @@ class Commit		@@ -6,15 +6,41 @@ class Commit
6		6
7	attr_mentionable :safe_message	7	attr_mentionable :safe_message
8		8
9	- # Safe amount of files with diffs in one commit to render	9	+ # Safe amount of changes (files and lines) in one commit to render
10	# Used to prevent 500 error on huge commits by suppressing diff	10	# Used to prevent 500 error on huge commits by suppressing diff
11	#	11	#
12	- DIFF_SAFE_SIZE = 100	12	+ # User can force display of diff above this size
		13	+ DIFF_SAFE_FILES = 100
		14	+ DIFF_SAFE_LINES = 5000
		15	+ # Commits above this size will not be rendered in HTML
		16	+ DIFF_HARD_LIMIT_FILES = 500
		17	+ DIFF_HARD_LIMIT_LINES = 10000
13		18
14	def self.decorate(commits)	19	def self.decorate(commits)
15	commits.map { \|c\| self.new(c) }	20	commits.map { \|c\| self.new(c) }
16	end	21	end
17		22
		23	+ # Calculate number of lines to render for diffs
		24	+ def self.diff_line_count(diffs)
		25	+ diffs.reduce(0){\|sum, d\| sum + d.diff.lines.count}
		26	+ end
		27	+
		28	+ def self.diff_suppress?(diffs, line_count = nil)
		29	+ # optimize - check file count first
		30	+ return true if diffs.size > DIFF_SAFE_FILES
		31	+
		32	+ line_count \|\|= Commit::diff_line_count(diffs)
		33	+ line_count > DIFF_SAFE_LINES
		34	+ end
		35	+
		36	+ def self.diff_force_suppress?(diffs, line_count = nil)
		37	+ # optimize - check file count first
		38	+ return true if diffs.size > DIFF_HARD_LIMIT_FILES
		39	+
		40	+ line_count \|\|= Commit::diff_line_count(diffs)
		41	+ line_count > DIFF_HARD_LIMIT_LINES
		42	+ end
		43	+
18	attr_accessor :raw	44	attr_accessor :raw
19		45
20	def initialize(raw_commit)	46	def initialize(raw_commit)
	@@ -27,6 +53,19 @@ class Commit		@@ -27,6 +53,19 @@ class Commit
27	@raw.id	53	@raw.id
28	end	54	end
29		55
		56	+ def diff_line_count
		57	+ @diff_line_count \|\|= Commit::diff_line_count(self.diffs)
		58	+ @diff_line_count
		59	+ end
		60	+
		61	+ def diff_suppress?
		62	+ Commit::diff_suppress?(self.diffs, diff_line_count)
		63	+ end
		64	+
		65	+ def diff_force_suppress?
		66	+ Commit::diff_force_suppress?(self.diffs, diff_line_count)
		67	+ end
		68	+
30	# Returns a string describing the commit for use in a link title	69	# Returns a string describing the commit for use in a link title
31	#	70	#
32	# Example	71	# Example
		1	+- @suppress_diff \|\|= @suppress_diff \|\| @force_suppress_diff
1	- if @suppress_diff	2	- if @suppress_diff
2	.alert.alert-block	3	.alert.alert-block
3	%p	4	%p
4	- %strong Warning! Large commit with more than #{Commit::DIFF_SAFE_SIZE} files changed.
5	- %p To preserve performance the diff is not shown.	5	+ %strong Warning! This is a large diff.
6	%p	6	%p
7	- But if you still want to see the diff
8	- = link_to "click this link", project_commit_path(project, @commit, force_show_diff: true), class: "underlined_link"	7	+ To preserve performance the diff is not shown.
		8	+ - if current_controller?(:commit) or current_controller?(:merge_requests)
		9	+ Please, download the diff as
		10	+ - if current_controller?(:commit)
		11	+ = link_to "plain diff", project_commit_path(@project, @commit, format: :diff), class: "underlined_link"
		12	+ or
		13	+ = link_to "email patch", project_commit_path(@project, @commit, format: :patch), class: "underlined_link"
		14	+ - else
		15	+ = link_to "plain diff", project_merge_request_path(@project, @merge_request, format: :diff), class: "underlined_link"
		16	+ or
		17	+ = link_to "email patch", project_merge_request_path(@project, @merge_request, format: :patch), class: "underlined_link"
		18	+ instead.
		19	+ - unless @force_suppress_diff
		20	+ %p
		21	+ If you still want to see the diff
		22	+ = link_to "click this link", url_for(force_show_diff: true), class: "underlined_link"
9		23
10	%p.commit-stat-summary	24	%p.commit-stat-summary
11	Showing	25	Showing
	@@ -27,3 +27,11 @@ Feature: Project Browse commits		@@ -27,3 +27,11 @@ Feature: Project Browse commits
27	Scenario: I browse commits stats	27	Scenario: I browse commits stats
28	Given I visit my project's commits stats page	28	Given I visit my project's commits stats page
29	Then I see commits stats	29	Then I see commits stats
		30	+
		31	+ Scenario: I browse big commit
		32	+ Given I visit big commit page
		33	+ Then I see big commit warning
		34	+
		35	+ Scenario: I browse huge commit
		36	+ Given I visit huge commit page
		37	+ Then I see huge commit message
	@@ -58,4 +58,24 @@ class ProjectBrowseCommits < Spinach::FeatureSteps		@@ -58,4 +58,24 @@ class ProjectBrowseCommits < Spinach::FeatureSteps
58	page.should have_content 'Total commits'	58	page.should have_content 'Total commits'
59	page.should have_content 'Authors'	59	page.should have_content 'Authors'
60	end	60	end
		61	+
		62	+ Given 'I visit big commit page' do
		63	+ visit project_commit_path(@project, BigCommits::BIG_COMMIT_ID)
		64	+ end
		65	+
		66	+ Then 'I see big commit warning' do
		67	+ page.should have_content BigCommits::BIG_COMMIT_MESSAGE
		68	+ page.should have_content "Warning! This is a large diff"
		69	+ page.should have_content "If you still want to see the diff"
		70	+ end
		71	+
		72	+ Given 'I visit huge commit page' do
		73	+ visit project_commit_path(@project, BigCommits::HUGE_COMMIT_ID)
		74	+ end
		75	+
		76	+ Then 'I see huge commit message' do
		77	+ page.should have_content BigCommits::HUGE_COMMIT_MESSAGE
		78	+ page.should have_content "Warning! This is a large diff"
		79	+ page.should_not have_content "If you still want to see the diff"
		80	+ end
61	end	81	end
	@@ -14,7 +14,7 @@ require 'spinach/capybara'		@@ -14,7 +14,7 @@ require 'spinach/capybara'
14	require 'sidekiq/testing/inline'	14	require 'sidekiq/testing/inline'
15		15
16		16
17	-%w(valid_commit select2_helper chosen_helper test_env).each do \|f\|	17	+%w(valid_commit big_commits select2_helper chosen_helper test_env).each do \|f\|
18	require Rails.root.join('spec', 'support', f)	18	require Rails.root.join('spec', 'support', f)
19	end	19	end
20		20
@@ -0,0 +1,8 @@		@@ -0,0 +1,8 @@
	1	+module BigCommits
	2	+ HUGE_COMMIT_ID = "7f92534f767fa20357a11c63f973ae3b79cc5b85"
	3	+ HUGE_COMMIT_MESSAGE = "pybments.rb version up. gitignore improved"
	4	+
	5	+ BIG_COMMIT_ID = "d62200cad430565bd9f80befaf329297120330b5"
	6	+ BIG_COMMIT_MESSAGE = "clean-up code"
	7	+end
	8	+