Commit 01ff084a authored by Boyan Tabakov's avatar Boyan Tabakov

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.
parent 71d31a38
...@@ -11,7 +11,7 @@ class MergeRequest ...@@ -11,7 +11,7 @@ class MergeRequest
constructor: (@opts) -> constructor: (@opts) ->
this.$el = $('.merge-request') this.$el = $('.merge-request')
@diffs_loaded = false @diffs_loaded = if @opts.action == 'diffs' then true else false
@commits_loaded = false @commits_loaded = false
this.activateTab(@opts.action) this.activateTab(@opts.action)
......
...@@ -20,7 +20,8 @@ class CommitLoadContext < BaseContext ...@@ -20,7 +20,8 @@ class CommitLoadContext < BaseContext
result[:notes_count] = project.notes.for_commit_id(commit.id).count result[:notes_count] = project.notes.for_commit_id(commit.id).count
begin 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 rescue Grit::Git::GitTimeout
result[:suppress_diff] = true result[:suppress_diff] = true
result[:status] = :huge_commit result[:status] = :huge_commit
......
...@@ -18,6 +18,7 @@ class Projects::CommitController < Projects::ApplicationController ...@@ -18,6 +18,7 @@ class Projects::CommitController < Projects::ApplicationController
end end
@suppress_diff = result[:suppress_diff] @suppress_diff = result[:suppress_diff]
@force_suppress_diff = result[:force_suppress_diff]
@note = result[:note] @note = result[:note]
@line_notes = result[:line_notes] @line_notes = result[:line_notes]
......
...@@ -15,6 +15,10 @@ class Projects::CompareController < Projects::ApplicationController ...@@ -15,6 +15,10 @@ class Projects::CompareController < Projects::ApplicationController
@diffs = compare.diffs @diffs = compare.diffs
@refs_are_same = compare.same @refs_are_same = compare.same
@line_notes = [] @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 end
def create def create
......
...@@ -40,6 +40,10 @@ class Projects::MergeRequestsController < Projects::ApplicationController ...@@ -40,6 +40,10 @@ class Projects::MergeRequestsController < Projects::ApplicationController
@comments_target = {noteable_type: 'MergeRequest', @comments_target = {noteable_type: 'MergeRequest',
noteable_id: @merge_request.id} noteable_id: @merge_request.id}
@line_notes = @merge_request.notes.where("line_code is not null") @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 end
def new def new
......
...@@ -6,15 +6,41 @@ class Commit ...@@ -6,15 +6,41 @@ class Commit
attr_mentionable :safe_message 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 # 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) def self.decorate(commits)
commits.map { |c| self.new(c) } commits.map { |c| self.new(c) }
end 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 attr_accessor :raw
def initialize(raw_commit) def initialize(raw_commit)
...@@ -27,6 +53,19 @@ class Commit ...@@ -27,6 +53,19 @@ class Commit
@raw.id @raw.id
end 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 # Returns a string describing the commit for use in a link title
# #
# Example # Example
......
- @suppress_diff ||= @suppress_diff || @force_suppress_diff
- if @suppress_diff - if @suppress_diff
.alert.alert-block .alert.alert-block
%p %p
%strong Warning! Large commit with more than #{Commit::DIFF_SAFE_SIZE} files changed. %strong Warning! This is a large diff.
%p To preserve performance the diff is not shown.
%p %p
But if you still want to see the diff To preserve performance the diff is not shown.
= link_to "click this link", project_commit_path(project, @commit, force_show_diff: true), class: "underlined_link" - 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 %p.commit-stat-summary
Showing Showing
......
...@@ -27,3 +27,11 @@ Feature: Project Browse commits ...@@ -27,3 +27,11 @@ Feature: Project Browse commits
Scenario: I browse commits stats Scenario: I browse commits stats
Given I visit my project's commits stats page Given I visit my project's commits stats page
Then I see commits stats 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 < Spinach::FeatureSteps ...@@ -58,4 +58,24 @@ class ProjectBrowseCommits < Spinach::FeatureSteps
page.should have_content 'Total commits' page.should have_content 'Total commits'
page.should have_content 'Authors' page.should have_content 'Authors'
end 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 end
...@@ -14,7 +14,7 @@ require 'spinach/capybara' ...@@ -14,7 +14,7 @@ require 'spinach/capybara'
require 'sidekiq/testing/inline' 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) require Rails.root.join('spec', 'support', f)
end end
......
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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment