From 1764e1b7cb2bffb9b4c4a69991fe2c4d21ce5459 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Thu, 3 Mar 2016 18:38:44 +0100
Subject: [PATCH] Use Gitlab::Git::DiffCollections
---
Gemfile | 2 +-
Gemfile.lock | 4 +-
app/controllers/projects/commit_controller.rb | 9 +-
.../projects/compare_controller.rb | 12 +--
.../projects/merge_requests_controller.rb | 4 +-
app/helpers/diff_helper.rb | 34 ++------
app/models/commit.rb | 16 ++--
app/models/merge_request.rb | 9 +-
app/models/merge_request_diff.rb | 86 +++++++++----------
app/models/note.rb | 26 +++---
app/services/compare_service.rb | 12 ++-
app/services/merge_requests/build_service.rb | 20 +----
app/views/projects/diffs/_diffs.html.haml | 11 +--
app/views/projects/diffs/_text_file.html.haml | 2 +-
app/views/projects/diffs/_warning.html.haml | 2 +-
.../merge_requests/_new_compare.html.haml | 29 +++----
.../merge_requests/_new_submit.html.haml | 10 +--
.../projects/merge_requests/_show.html.haml | 2 +-
.../merge_requests/show/_diffs.html.haml | 2 +-
.../search/results/_snippet_blob.html.haml | 2 +-
app/workers/irker_worker.rb | 2 +-
...58_add_real_size_to_merge_request_diffs.rb | 5 ++
db/schema.rb | 1 +
features/steps/project/commits/commits.rb | 7 +-
lib/api/commits.rb | 6 +-
lib/api/entities.rb | 6 +-
lib/gitlab/compare_result.rb | 9 --
lib/gitlab/diff/file.rb | 2 +-
lib/gitlab/diff/parser.rb | 79 ++++++++---------
lib/gitlab/email/message/repository_push.rb | 2 +-
spec/controllers/commit_controller_spec.rb | 2 +-
.../projects/compare_controller_spec.rb | 8 +-
spec/helpers/diff_helper_spec.rb | 57 +-----------
spec/lib/gitlab/diff/parser_spec.rb | 2 +-
.../email/message/repository_push_spec.rb | 2 +-
.../merge_requests/refresh_service_spec.rb | 2 +-
36 files changed, 192 insertions(+), 294 deletions(-)
create mode 100644 db/migrate/20160204144558_add_real_size_to_merge_request_diffs.rb
delete mode 100644 lib/gitlab/compare_result.rb
diff --git a/Gemfile b/Gemfile
index 134646cf800..283e544ee9f 100644
--- a/Gemfile
+++ b/Gemfile
@@ -50,7 +50,7 @@ gem "browser", '~> 1.0.0'
# Extracting information from a git repository
# Provide access to Gitlab::Git library
-gem "gitlab_git", '~> 8.2'
+gem "gitlab_git", '~> 9.0'
# LDAP Auth
# GitLab fork with several improvements to original library. For full list of changes
diff --git a/Gemfile.lock b/Gemfile.lock
index e048e2f5a56..79b80c1fa35 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -357,7 +357,7 @@ GEM
posix-spawn (~> 0.3)
gitlab_emoji (0.3.1)
gemojione (~> 2.2, >= 2.2.1)
- gitlab_git (8.2.0)
+ gitlab_git (9.0.0)
activesupport (~> 4.0)
charlock_holmes (~> 0.7.3)
github-linguist (~> 4.7.0)
@@ -929,7 +929,7 @@ DEPENDENCIES
github-markup (~> 1.3.1)
gitlab-flowdock-git-hook (~> 1.0.1)
gitlab_emoji (~> 0.3.0)
- gitlab_git (~> 8.2)
+ gitlab_git (~> 9.0)
gitlab_meta (= 7.0)
gitlab_omniauth-ldap (~> 1.2.1)
gollum-lib (~> 4.1.0)
diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb
index 97d31a4229a..576fa3cedb2 100644
--- a/app/controllers/projects/commit_controller.rb
+++ b/app/controllers/projects/commit_controller.rb
@@ -3,6 +3,7 @@
# Not to be confused with CommitsController, plural.
class Projects::CommitController < Projects::ApplicationController
include CreatesCommit
+ include DiffHelper
# Authorize
before_action :require_non_empty_project
@@ -100,12 +101,10 @@ class Projects::CommitController < Projects::ApplicationController
def define_show_vars
return git_not_found! unless commit
- if params[:w].to_i == 1
- @diffs = commit.diffs({ ignore_whitespace_change: true })
- else
- @diffs = commit.diffs
- end
+ opts = diff_options
+ opts[:ignore_whitespace_change] = true if params[:format] == 'diff'
+ @diffs = commit.diffs(opts)
@diff_refs = [commit.parent || commit, commit]
@notes_count = commit.notes.count
diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb
index dc5d217f3e4..671d5c23024 100644
--- a/app/controllers/projects/compare_controller.rb
+++ b/app/controllers/projects/compare_controller.rb
@@ -1,6 +1,8 @@
require 'addressable/uri'
class Projects::CompareController < Projects::ApplicationController
+ include DiffHelper
+
# Authorize
before_action :require_non_empty_project
before_action :authorize_download_code!
@@ -11,16 +13,14 @@ class Projects::CompareController < Projects::ApplicationController
end
def show
- diff_options = { ignore_whitespace_change: true } if params[:w] == '1'
-
- compare_result = CompareService.new.
+ compare = CompareService.new.
execute(@project, @head_ref, @project, @base_ref, diff_options)
- if compare_result
- @commits = Commit.decorate(compare_result.commits, @project)
- @diffs = compare_result.diffs
+ if compare
+ @commits = Commit.decorate(compare.commits, @project)
@commit = @project.commit(@head_ref)
@base_commit = @project.merge_base_commit(@base_ref, @head_ref)
+ @diffs = compare.diffs(diff_options)
@diff_refs = [@base_commit, @commit]
@line_notes = []
end
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 5fe21694605..03ba289eb94 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -1,4 +1,6 @@
class Projects::MergeRequestsController < Projects::ApplicationController
+ include DiffHelper
+
before_action :module_enabled
before_action :merge_request, only: [
:edit, :update, :show, :diffs, :commits, :builds, :merge, :merge_check,
@@ -111,7 +113,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
@commits = @merge_request.compare_commits.reverse
@commit = @merge_request.last_commit
@base_commit = @merge_request.diff_base_commit
- @diffs = @merge_request.compare_diffs
+ @diffs = @merge_request.compare.diffs(diff_options) if @merge_request.compare
@ci_commit = @merge_request.ci_commit
@statuses = @ci_commit.statuses if @ci_commit
diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb
index d76db867c5a..ff32e834499 100644
--- a/app/helpers/diff_helper.rb
+++ b/app/helpers/diff_helper.rb
@@ -12,40 +12,20 @@ module DiffHelper
params[:view] == 'parallel' ? 'parallel' : 'inline'
end
- def allowed_diff_size
- if diff_hard_limit_enabled?
- Commit::DIFF_HARD_LIMIT_FILES
- else
- Commit::DIFF_SAFE_FILES
- end
+ def diff_hard_limit_enabled?
+ params[:force_show_diff].present?
end
- def allowed_diff_lines
+ def diff_options
+ options = { ignore_whitespace_change: params[:w] == '1' }
if diff_hard_limit_enabled?
- Commit::DIFF_HARD_LIMIT_LINES
- else
- Commit::DIFF_SAFE_LINES
+ options.merge!(Commit.max_diff_options)
end
+ options
end
def safe_diff_files(diffs, diff_refs)
- lines = 0
- safe_files = []
- diffs.first(allowed_diff_size).each do |diff|
- lines += diff.diff.lines.count
- break if lines > allowed_diff_lines
- safe_files << Gitlab::Diff::File.new(diff, diff_refs)
- end
- safe_files
- end
-
- def diff_hard_limit_enabled?
- # Enabling hard limit allows user to see more diff information
- if params[:force_show_diff].present?
- true
- else
- false
- end
+ diffs.decorate! { |diff| Gitlab::Diff::File.new(diff, diff_refs) }
end
def generate_line_code(file_path, line)
diff --git a/app/models/commit.rb b/app/models/commit.rb
index 3224f5457f0..ce0b85d50cf 100644
--- a/app/models/commit.rb
+++ b/app/models/commit.rb
@@ -12,12 +12,7 @@ class Commit
attr_accessor :project
- # Safe amount of changes (files and lines) in one commit to render
- # Used to prevent 500 error on huge commits by suppressing diff
- #
- # User can force display of diff above this size
- DIFF_SAFE_FILES = 100 unless defined?(DIFF_SAFE_FILES)
- DIFF_SAFE_LINES = 5000 unless defined?(DIFF_SAFE_LINES)
+ DIFF_SAFE_LINES = Gitlab::Git::DiffCollection::DEFAULT_LIMITS[:max_lines]
# Commits above this size will not be rendered in HTML
DIFF_HARD_LIMIT_FILES = 1000 unless defined?(DIFF_HARD_LIMIT_FILES)
@@ -36,13 +31,20 @@ class Commit
# Calculate number of lines to render for diffs
def diff_line_count(diffs)
- diffs.reduce(0) { |sum, d| sum + d.diff.lines.count }
+ diffs.reduce(0) { |sum, d| sum + Gitlab::Git::Util.count_lines(d.diff) }
end
# Truncate sha to 8 characters
def truncate_sha(sha)
sha[0..7]
end
+
+ def max_diff_options
+ {
+ max_files: DIFF_HARD_LIMIT_FILES,
+ max_lines: DIFF_HARD_LIMIT_LINES,
+ }
+ end
end
attr_accessor :raw
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 1543ef311d7..025b522cf66 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -48,7 +48,7 @@ class MergeRequest < ActiveRecord::Base
after_create :create_merge_request_diff
after_update :update_merge_request_diff
- delegate :commits, :diffs, :diffs_no_whitespace, to: :merge_request_diff, prefix: nil
+ delegate :commits, :diffs, :real_size, to: :merge_request_diff, prefix: nil
# When this attribute is true some MR validation is ignored
# It allows us to close or modify broken merge requests
@@ -56,8 +56,7 @@ class MergeRequest < ActiveRecord::Base
# Temporary fields to store compare vars
# when creating new merge request
- attr_accessor :can_be_created, :compare_failed,
- :compare_commits, :compare_diffs
+ attr_accessor :can_be_created, :compare_commits, :compare
state_machine :state, initial: :opened do
event :close do
@@ -182,6 +181,10 @@ class MergeRequest < ActiveRecord::Base
merge_request_diff ? merge_request_diff.first_commit : compare_commits.first
end
+ def diff_size
+ merge_request_diff.size
+ end
+
def diff_base_commit
if merge_request_diff
merge_request_diff.base_commit
diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb
index c95179d6046..df08d3a6dfb 100644
--- a/app/models/merge_request_diff.rb
+++ b/app/models/merge_request_diff.rb
@@ -19,14 +19,15 @@ class MergeRequestDiff < ActiveRecord::Base
# Prevent store of diff if commits amount more then 500
COMMITS_SAFE_SIZE = 500
- attr_reader :commits, :diffs, :diffs_no_whitespace
-
belongs_to :merge_request
delegate :target_branch, :source_branch, to: :merge_request, prefix: nil
state_machine :state, initial: :empty do
state :collected
+ state :overflow
+ # Deprecated states: these are no longer used but these values may still occur
+ # in the database.
state :timeout
state :overflow_commits_safe_size
state :overflow_diff_files_limit
@@ -43,19 +44,23 @@ class MergeRequestDiff < ActiveRecord::Base
reload_diffs
end
- def diffs
- @diffs ||= (load_diffs(st_diffs) || [])
+ def size
+ real_size.presence || diffs.size
end
- def diffs_no_whitespace
- compare_result = Gitlab::CompareResult.new(
- Gitlab::Git::Compare.new(
- self.repository.raw_repository,
- self.target_branch,
- self.source_sha,
- ), { ignore_whitespace_change: true }
- )
- @diffs_no_whitespace ||= load_diffs(dump_commits(compare_result.diffs))
+ def diffs(options={})
+ if options[:ignore_whitespace_change]
+ @diffs_no_whitespace ||= begin
+ compare = Gitlab::Git::Compare.new(
+ self.repository.raw_repository,
+ self.target_branch,
+ self.source_sha,
+ )
+ compare.diffs(options)
+ end
+ else
+ @diffs ||= load_diffs(st_diffs, options)
+ end
end
def commits
@@ -94,16 +99,18 @@ class MergeRequestDiff < ActiveRecord::Base
end
end
- def load_diffs(raw)
- if raw.respond_to?(:map)
- raw.map { |hash| Gitlab::Git::Diff.new(hash) }
+ def load_diffs(raw, options)
+ if raw.respond_to?(:each)
+ Gitlab::Git::DiffCollection.new(raw, options)
+ else
+ Gitlab::Git::DiffCollection.new([])
end
end
# Collect array of Git::Commit objects
# between target and source branches
def unmerged_commits
- commits = compare_result.commits
+ commits = compare.commits
if commits.present?
commits = Commit.decorate(commits, merge_request.source_project).
@@ -133,27 +140,21 @@ class MergeRequestDiff < ActiveRecord::Base
if commits.size.zero?
self.state = :empty
- elsif commits.size > COMMITS_SAFE_SIZE
- self.state = :overflow_commits_safe_size
else
- new_diffs = unmerged_diffs
- end
+ diff_collection = unmerged_diffs
- if new_diffs.any?
- if new_diffs.size > Commit::DIFF_HARD_LIMIT_FILES
- self.state = :overflow_diff_files_limit
- new_diffs = new_diffs.first(Commit::DIFF_HARD_LIMIT_LINES)
+ if diff_collection.overflow?
+ # Set our state to 'overflow' to make the #empty? and #collected?
+ # methods (generated by StateMachine) return false.
+ self.state = :overflow
end
- if new_diffs.sum { |diff| diff.diff.lines.count } > Commit::DIFF_HARD_LIMIT_LINES
- self.state = :overflow_diff_lines_limit
- new_diffs = new_diffs.first(Commit::DIFF_HARD_LIMIT_LINES)
- end
- end
+ self.real_size = diff_collection.real_size
- if new_diffs.present?
- new_diffs = dump_commits(new_diffs)
- self.state = :collected
+ if diff_collection.any?
+ new_diffs = dump_diffs(diff_collection)
+ self.state = :collected
+ end
end
self.st_diffs = new_diffs
@@ -166,10 +167,7 @@ class MergeRequestDiff < ActiveRecord::Base
# Collect array of Git::Diff objects
# between target and source branches
def unmerged_diffs
- compare_result.diffs || []
- rescue Gitlab::Git::Diff::TimeoutError
- self.state = :timeout
- []
+ compare.diffs(Commit.max_diff_options)
end
def repository
@@ -181,18 +179,16 @@ class MergeRequestDiff < ActiveRecord::Base
source_commit.try(:sha)
end
- def compare_result
- @compare_result ||=
+ def compare
+ @compare ||=
begin
# Update ref for merge request
merge_request.fetch_ref
- Gitlab::CompareResult.new(
- Gitlab::Git::Compare.new(
- self.repository.raw_repository,
- self.target_branch,
- self.source_sha
- )
+ Gitlab::Git::Compare.new(
+ self.repository.raw_repository,
+ self.target_branch,
+ self.source_sha
)
end
end
diff --git a/app/models/note.rb b/app/models/note.rb
index d287e0f3c6d..1a7b2ba6d42 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -131,9 +131,11 @@ class Note < ActiveRecord::Base
end
def find_diff
- return nil unless noteable && noteable.diffs.present?
+ return nil unless noteable
+ return @diff if defined?(@diff)
- @diff ||= noteable.diffs.find do |d|
+ # Don't use ||= because nil is a valid value for @diff
+ @diff = noteable.diffs(Commit.max_diff_options).find do |d|
Digest::SHA1.hexdigest(d.new_path) == diff_file_index if d.new_path
end
end
@@ -165,20 +167,16 @@ class Note < ActiveRecord::Base
def active?
return true unless self.diff
return false unless noteable
+ return @active if defined?(@active)
- noteable.diffs.each do |mr_diff|
- next unless mr_diff.new_path == self.diff.new_path
+ diffs = noteable.diffs(Commit.max_diff_options)
+ notable_diff = diffs.find { |d| d.new_path == self.diff.new_path }
- lines = Gitlab::Diff::Parser.new.parse(mr_diff.diff.lines.to_a)
+ return @active = false if notable_diff.nil?
- lines.each do |line|
- if line.text == diff_line
- return true
- end
- end
- end
-
- false
+ parsed_lines = Gitlab::Diff::Parser.new.parse(notable_diff.diff.each_line)
+ # We cannot use ||= because @active may be false
+ @active = parsed_lines.any? { |line_obj| line_obj.text == diff_line }
end
def outdated?
@@ -263,7 +261,7 @@ class Note < ActiveRecord::Base
end
def diff_lines
- @diff_lines ||= Gitlab::Diff::Parser.new.parse(diff.diff.lines)
+ @diff_lines ||= Gitlab::Diff::Parser.new.parse(diff.diff.each_line)
end
def highlighted_diff_lines
diff --git a/app/services/compare_service.rb b/app/services/compare_service.rb
index ec581658fc1..e2bccbdbcc3 100644
--- a/app/services/compare_service.rb
+++ b/app/services/compare_service.rb
@@ -1,7 +1,7 @@
require 'securerandom'
# Compare 2 branches for one repo or between repositories
-# and return Gitlab::CompareResult object that responds to commits and diffs
+# and return Gitlab::Git::Compare object that responds to commits and diffs
class CompareService
def execute(source_project, source_branch, target_project, target_branch, diff_options = {})
source_commit = source_project.commit(source_branch)
@@ -20,12 +20,10 @@ class CompareService
)
end
- Gitlab::CompareResult.new(
- Gitlab::Git::Compare.new(
- target_project.repository.raw_repository,
- target_branch,
- source_sha,
- ), diff_options
+ Gitlab::Git::Compare.new(
+ target_project.repository.raw_repository,
+ target_branch,
+ source_sha,
)
end
end
diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb
index c0700d953dd..954746a39a5 100644
--- a/app/services/merge_requests/build_service.rb
+++ b/app/services/merge_requests/build_service.rb
@@ -5,9 +5,7 @@ module MergeRequests
# Set MR attributes
merge_request.can_be_created = false
- merge_request.compare_failed = false
merge_request.compare_commits = []
- merge_request.compare_diffs = []
merge_request.source_project = project unless merge_request.source_project
merge_request.target_project ||= (project.forked_from_project || project)
merge_request.target_branch ||= merge_request.target_project.default_branch
@@ -21,35 +19,23 @@ module MergeRequests
return build_failed(merge_request, message)
end
- compare_result = CompareService.new.execute(
+ compare = CompareService.new.execute(
merge_request.source_project,
merge_request.source_branch,
merge_request.target_project,
merge_request.target_branch,
)
- commits = compare_result.commits
+ commits = compare.commits
# At this point we decide if merge request can be created
# If we have at least one commit to merge -> creation allowed
if commits.present?
merge_request.compare_commits = Commit.decorate(commits, merge_request.source_project)
merge_request.can_be_created = true
- merge_request.compare_failed = false
-
- # Try to collect diff for merge request.
- diffs = compare_result.diffs
-
- if diffs.present?
- merge_request.compare_diffs = diffs
-
- elsif diffs == false
- merge_request.can_be_created = false
- merge_request.compare_failed = true
- end
+ merge_request.compare = compare
else
merge_request.can_be_created = false
- merge_request.compare_failed = false
end
commits = merge_request.compare_commits
diff --git a/app/views/projects/diffs/_diffs.html.haml b/app/views/projects/diffs/_diffs.html.haml
index d668f483bcb..6086ad3661e 100644
--- a/app/views/projects/diffs/_diffs.html.haml
+++ b/app/views/projects/diffs/_diffs.html.haml
@@ -10,8 +10,8 @@
= parallel_diff_btn
= render 'projects/diffs/stats', diff_files: diff_files
-- if diff_files.count < diffs.size
- = render 'projects/diffs/warning', diffs: diffs, shown_files_count: diff_files.count
+- if diff_files.overflow?
+ = render 'projects/diffs/warning', diff_files: diff_files
.files
- diff_files.each_with_index do |diff_file, index|
@@ -21,10 +21,3 @@
= render 'projects/diffs/file', i: index, project: project,
diff_file: diff_file, diff_commit: diff_commit, blob: blob
-
-- if @diff_timeout
- .alert.alert-danger
- %h4
- Failed to collect changes
- %p
- Maybe diff is really big and operation failed with timeout. Try to get diff locally
diff --git a/app/views/projects/diffs/_text_file.html.haml b/app/views/projects/diffs/_text_file.html.haml
index d75e9ef2a49..9a8208202e4 100644
--- a/app/views/projects/diffs/_text_file.html.haml
+++ b/app/views/projects/diffs/_text_file.html.haml
@@ -6,7 +6,7 @@
%table.text-file.code.js-syntax-highlight{ class: too_big ? 'hide' : '' }
- last_line = 0
- - raw_diff_lines = diff_file.diff_lines
+ - raw_diff_lines = diff_file.diff_lines.to_a
- diff_file.highlighted_diff_lines.each_with_index do |line, index|
- type = line.type
- last_line = line.new_pos
diff --git a/app/views/projects/diffs/_warning.html.haml b/app/views/projects/diffs/_warning.html.haml
index 63ede71e6f1..15536c17f8e 100644
--- a/app/views/projects/diffs/_warning.html.haml
+++ b/app/views/projects/diffs/_warning.html.haml
@@ -14,5 +14,5 @@
= link_to "Email patch", merge_request_path(@merge_request, format: :patch), class: "btn btn-sm"
%p
To preserve performance only
- %strong #{shown_files_count} of #{diffs.size}
+ %strong #{diff_files.count} of #{diff_files.real_size}
files are displayed.
diff --git a/app/views/projects/merge_requests/_new_compare.html.haml b/app/views/projects/merge_requests/_new_compare.html.haml
index 236a545c840..01dc7519bee 100644
--- a/app/views/projects/merge_requests/_new_compare.html.haml
+++ b/app/views/projects/merge_requests/_new_compare.html.haml
@@ -33,23 +33,18 @@
%div= msg
- elsif @merge_request.source_branch.present? && @merge_request.target_branch.present?
- - if @merge_request.compare_failed
- .alert.alert-danger
- %h4 Compare failed
- %p We can't compare selected branches. It may be because of huge diff. Please try again or select different branches.
- - else
- .light-well.append-bottom-default
- .center
- %h4
- There isn't anything to merge.
- %p.slead
- - if @merge_request.source_branch == @merge_request.target_branch
- You'll need to use different branch names to get a valid comparison.
- - else
- %span.label-branch #{@merge_request.source_branch}
- and
- %span.label-branch #{@merge_request.target_branch}
- are the same.
+ .light-well.append-bottom-default
+ .center
+ %h4
+ There isn't anything to merge.
+ %p.slead
+ - if @merge_request.source_branch == @merge_request.target_branch
+ You'll need to use different branch names to get a valid comparison.
+ - else
+ %span.label-branch #{@merge_request.source_branch}
+ and
+ %span.label-branch #{@merge_request.target_branch}
+ are the same.
.form-actions
diff --git a/app/views/projects/merge_requests/_new_submit.html.haml b/app/views/projects/merge_requests/_new_submit.html.haml
index 4c5a9818e3e..9e59f7df71b 100644
--- a/app/views/projects/merge_requests/_new_submit.html.haml
+++ b/app/views/projects/merge_requests/_new_submit.html.haml
@@ -31,22 +31,18 @@
%li.diffs-tab.active
= link_to url_for(params), data: {target: 'div#diffs', action: 'diffs', toggle: 'tab'} do
Changes
- %span.badge= @diffs.size
+ %span.badge= @diffs.real_size
.tab-content
#commits.commits.tab-pane
= render "projects/merge_requests/show/commits"
#diffs.diffs.tab-pane.active
- - if @diffs.present?
- = render "projects/diffs/diffs", diffs: @diffs, project: @project, diff_refs: @merge_request.diff_refs
- - elsif @commits.size > MergeRequestDiff::COMMITS_SAFE_SIZE
+ - if @commits.size > MergeRequestDiff::COMMITS_SAFE_SIZE
.alert.alert-danger
%h4 This comparison includes more than #{MergeRequestDiff::COMMITS_SAFE_SIZE} commits.
%p To preserve performance the line changes are not shown.
- else
- .alert.alert-danger
- %h4 This comparison includes a huge diff.
- %p To preserve performance the line changes are not shown.
+ = render "projects/diffs/diffs", diffs: @diffs, project: @project, diff_refs: @merge_request.diff_refs
- if @ci_commit
#builds.builds.tab-pane
= render "projects/merge_requests/show/builds"
diff --git a/app/views/projects/merge_requests/_show.html.haml b/app/views/projects/merge_requests/_show.html.haml
index d7bc26e24b9..7b5e2991c09 100644
--- a/app/views/projects/merge_requests/_show.html.haml
+++ b/app/views/projects/merge_requests/_show.html.haml
@@ -62,7 +62,7 @@
%li.diffs-tab
= link_to diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), data: {target: 'div#diffs', action: 'diffs', toggle: 'tab'} do
Changes
- %span.badge= @merge_request.diffs.size
+ %span.badge= @merge_request.diff_size
.tab-content
#notes.notes.tab-pane.voting_notes
diff --git a/app/views/projects/merge_requests/show/_diffs.html.haml b/app/views/projects/merge_requests/show/_diffs.html.haml
index 64cd484193e..1b0bae86ad4 100644
--- a/app/views/projects/merge_requests/show/_diffs.html.haml
+++ b/app/views/projects/merge_requests/show/_diffs.html.haml
@@ -1,5 +1,5 @@
- if @merge_request_diff.collected?
- = render "projects/diffs/diffs", diffs: params[:w] == '1' ? @merge_request.diffs_no_whitespace : @merge_request.diffs,
+ = render "projects/diffs/diffs", diffs: @merge_request.diffs(diff_options),
project: @merge_request.project, diff_refs: @merge_request.diff_refs
- elsif @merge_request_diff.empty?
.nothing-here-block Nothing to merge from #{@merge_request.source_branch} into #{@merge_request.target_branch}
diff --git a/app/views/search/results/_snippet_blob.html.haml b/app/views/search/results/_snippet_blob.html.haml
index 6b77d24f50c..c9b7bd154af 100644
--- a/app/views/search/results/_snippet_blob.html.haml
+++ b/app/views/search/results/_snippet_blob.html.haml
@@ -30,7 +30,7 @@
.line-numbers
- snippet_chunks.each do |chunk|
- unless chunk[:data].empty?
- - chunk[:data].lines.to_a.size.times do |index|
+ - Gitlab::Git::Util.count_lines(chunk[:data]).times do |index|
- offset = defined?(chunk[:start_line]) ? chunk[:start_line] : 1
- i = index + offset
= link_to snippet_path+"#L#{i}", id: "L#{i}", rel: "#L#{i}", class: "diff-line-num" do
diff --git a/app/workers/irker_worker.rb b/app/workers/irker_worker.rb
index 2d44d8d4dc6..605ec4f04e5 100644
--- a/app/workers/irker_worker.rb
+++ b/app/workers/irker_worker.rb
@@ -141,7 +141,7 @@ class IrkerWorker
end
def files_count(commit)
- files = "#{commit.diffs.count} file"
+ files = "#{commit.diffs.real_size} file"
files += 's' if commit.diffs.count > 1
files
end
diff --git a/db/migrate/20160204144558_add_real_size_to_merge_request_diffs.rb b/db/migrate/20160204144558_add_real_size_to_merge_request_diffs.rb
new file mode 100644
index 00000000000..f996ae74dca
--- /dev/null
+++ b/db/migrate/20160204144558_add_real_size_to_merge_request_diffs.rb
@@ -0,0 +1,5 @@
+class AddRealSizeToMergeRequestDiffs < ActiveRecord::Migration
+ def change
+ add_column :merge_request_diffs, :real_size, :string
+ end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 53a941d30de..71d9257a31e 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -509,6 +509,7 @@ ActiveRecord::Schema.define(version: 20160222153918) do
t.datetime "created_at"
t.datetime "updated_at"
t.string "base_commit_sha"
+ t.string "real_size"
end
add_index "merge_request_diffs", ["merge_request_id"], name: "index_merge_request_diffs_on_merge_request_id", unique: true, using: :btree
diff --git a/features/steps/project/commits/commits.rb b/features/steps/project/commits/commits.rb
index f9fd7332464..93c37bf507f 100644
--- a/features/steps/project/commits/commits.rb
+++ b/features/steps/project/commits/commits.rb
@@ -126,8 +126,11 @@ class Spinach::Features::ProjectCommits < Spinach::FeatureSteps
end
step 'I visit big commit page' do
- stub_const('Commit::DIFF_SAFE_FILES', 20)
- visit namespace_project_commit_path(@project.namespace, @project, sample_big_commit.id)
+ # Create a temporary scope to ensure that the stub_const is removed after user
+ RSpec::Mocks.with_temporary_scope do
+ stub_const('Gitlab::Git::DiffCollection::DEFAULT_LIMITS', { max_lines: 1, max_files: 1 })
+ visit namespace_project_commit_path(@project.namespace, @project, sample_big_commit.id)
+ end
end
step 'I see big commit warning' do
diff --git a/lib/api/commits.rb b/lib/api/commits.rb
index f4efb651eb6..4544a41b1e3 100644
--- a/lib/api/commits.rb
+++ b/lib/api/commits.rb
@@ -48,7 +48,7 @@ module API
sha = params[:sha]
commit = user_project.commit(sha)
not_found! "Commit" unless commit
- commit.diffs
+ commit.diffs.to_a
end
# Get a commit's comments
@@ -90,9 +90,9 @@ module API
}
if params[:path] && params[:line] && params[:line_type]
- commit.diffs.each do |diff|
+ commit.diffs(all_diffs: true).each do |diff|
next unless diff.new_path == params[:path]
- lines = Gitlab::Diff::Parser.new.parse(diff.diff.lines.to_a)
+ lines = Gitlab::Diff::Parser.new.parse(diff.diff.each_line)
lines.each do |line|
next unless line.new_pos == params[:line].to_i && line.type == params[:line_type]
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index a3b5f1eb8d3..b021db8fa5b 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -181,7 +181,7 @@ module API
class MergeRequestChanges < MergeRequest
expose :diffs, as: :changes, using: Entities::RepoDiff do |compare, _|
- compare.diffs
+ compare.diffs(all_diffs: true).to_a
end
end
@@ -295,11 +295,11 @@ module API
end
expose :diffs, using: Entities::RepoDiff do |compare, options|
- compare.diffs
+ compare.diffs(all_diffs: true).to_a
end
expose :compare_timeout do |compare, options|
- compare.timeout
+ compare.diffs.overflow?
end
expose :same, as: :compare_same_ref
diff --git a/lib/gitlab/compare_result.rb b/lib/gitlab/compare_result.rb
deleted file mode 100644
index 0d696a1ee28..00000000000
--- a/lib/gitlab/compare_result.rb
+++ /dev/null
@@ -1,9 +0,0 @@
-module Gitlab
- class CompareResult
- attr_reader :commits, :diffs
-
- def initialize(compare, diff_options = {})
- @commits, @diffs = compare.commits, compare.diffs(nil, diff_options)
- end
- end
-end
diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb
index a484177ae33..faa2830c16e 100644
--- a/lib/gitlab/diff/file.rb
+++ b/lib/gitlab/diff/file.rb
@@ -21,7 +21,7 @@ module Gitlab
# Array of Gitlab::DIff::Line objects
def diff_lines
- @lines ||= parser.parse(raw_diff.lines)
+ @lines ||= parser.parse(raw_diff.each_line).to_a
end
def highlighted_diff_lines
diff --git a/lib/gitlab/diff/parser.rb b/lib/gitlab/diff/parser.rb
index 3666063bf8b..d0f6ba23ab4 100644
--- a/lib/gitlab/diff/parser.rb
+++ b/lib/gitlab/diff/parser.rb
@@ -5,52 +5,53 @@ module Gitlab
def parse(lines)
@lines = lines
- lines_obj = []
line_obj_index = 0
line_old = 1
line_new = 1
type = nil
- @lines.each do |line|
- next if filename?(line)
-
- full_line = line.gsub(/\n/, '')
-
- if line.match(/^@@ -/)
- type = "match"
-
- line_old = line.match(/\-[0-9]*/)[0].to_i.abs rescue 0
- line_new = line.match(/\+[0-9]*/)[0].to_i.abs rescue 0
-
- next if line_old <= 1 && line_new <= 1 #top of file
- lines_obj << Gitlab::Diff::Line.new(full_line, type, line_obj_index, line_old, line_new)
- line_obj_index += 1
- next
- elsif line[0] == '\\'
- type = 'nonewline'
- lines_obj << Gitlab::Diff::Line.new(full_line, type, line_obj_index, line_old, line_new)
- line_obj_index += 1
- else
- type = identification_type(line)
- lines_obj << Gitlab::Diff::Line.new(full_line, type, line_obj_index, line_old, line_new)
- line_obj_index += 1
- end
-
-
- case line[0]
- when "+"
- line_new += 1
- when "-"
- line_old += 1
- when "\\"
- # No increment
- else
- line_new += 1
- line_old += 1
+ # By returning an Enumerator we make it possible to search for a single line (with #find)
+ # without having to instantiate all the others that come after it.
+ Enumerator.new do |yielder|
+ @lines.each do |line|
+ next if filename?(line)
+
+ full_line = line.gsub(/\n/, '')
+
+ if line.match(/^@@ -/)
+ type = "match"
+
+ line_old = line.match(/\-[0-9]*/)[0].to_i.abs rescue 0
+ line_new = line.match(/\+[0-9]*/)[0].to_i.abs rescue 0
+
+ next if line_old <= 1 && line_new <= 1 #top of file
+ yielder << Gitlab::Diff::Line.new(full_line, type, line_obj_index, line_old, line_new)
+ line_obj_index += 1
+ next
+ elsif line[0] == '\\'
+ type = 'nonewline'
+ yielder << Gitlab::Diff::Line.new(full_line, type, line_obj_index, line_old, line_new)
+ line_obj_index += 1
+ else
+ type = identification_type(line)
+ yielder << Gitlab::Diff::Line.new(full_line, type, line_obj_index, line_old, line_new)
+ line_obj_index += 1
+ end
+
+
+ case line[0]
+ when "+"
+ line_new += 1
+ when "-"
+ line_old += 1
+ when "\\"
+ # No increment
+ else
+ line_new += 1
+ line_old += 1
+ end
end
end
-
- lines_obj
end
def empty?
diff --git a/lib/gitlab/email/message/repository_push.rb b/lib/gitlab/email/message/repository_push.rb
index a05ffeb9cd2..41f0edcaf7e 100644
--- a/lib/gitlab/email/message/repository_push.rb
+++ b/lib/gitlab/email/message/repository_push.rb
@@ -50,7 +50,7 @@ module Gitlab
end
def compare_timeout
- compare.timeout if compare
+ diffs.overflow? if diffs
end
def reverse_compare?
diff --git a/spec/controllers/commit_controller_spec.rb b/spec/controllers/commit_controller_spec.rb
index bbe400dad88..f09e4fcb154 100644
--- a/spec/controllers/commit_controller_spec.rb
+++ b/spec/controllers/commit_controller_spec.rb
@@ -81,7 +81,7 @@ describe Projects::CommitController do
expect(response.body).to start_with("diff --git")
# without whitespace option, there are more than 2 diff_splits
- diff_splits = assigns(:diffs)[0].diff.split("\n")
+ diff_splits = assigns(:diffs).first.diff.split("\n")
expect(diff_splits.length).to be <= 2
end
end
diff --git a/spec/controllers/projects/compare_controller_spec.rb b/spec/controllers/projects/compare_controller_spec.rb
index be19f1abc53..788a609ee40 100644
--- a/spec/controllers/projects/compare_controller_spec.rb
+++ b/spec/controllers/projects/compare_controller_spec.rb
@@ -19,7 +19,7 @@ describe Projects::CompareController do
to: ref_to)
expect(response).to be_success
- expect(assigns(:diffs).length).to be >= 1
+ expect(assigns(:diffs).first).to_not be_nil
expect(assigns(:commits).length).to be >= 1
end
@@ -32,10 +32,10 @@ describe Projects::CompareController do
w: 1)
expect(response).to be_success
- expect(assigns(:diffs).length).to be >= 1
+ expect(assigns(:diffs).first).to_not be_nil
expect(assigns(:commits).length).to be >= 1
# without whitespace option, there are more than 2 diff_splits
- diff_splits = assigns(:diffs)[0].diff.split("\n")
+ diff_splits = assigns(:diffs).first.diff.split("\n")
expect(diff_splits.length).to be <= 2
end
@@ -48,7 +48,7 @@ describe Projects::CompareController do
to: ref_to)
expect(response).to be_success
- expect(assigns(:diffs)).to eq([])
+ expect(assigns(:diffs).to_a).to eq([])
expect(assigns(:commits)).to eq([])
end
diff --git a/spec/helpers/diff_helper_spec.rb b/spec/helpers/diff_helper_spec.rb
index 14986a74c2e..982c113e84b 100644
--- a/spec/helpers/diff_helper_spec.rb
+++ b/spec/helpers/diff_helper_spec.rb
@@ -22,65 +22,14 @@ describe DiffHelper do
end
end
- describe 'allowed_diff_size' do
+ describe 'diff_options' do
it 'should return hard limit for a diff if force diff is true' do
allow(controller).to receive(:params) { { force_show_diff: true } }
- expect(allowed_diff_size).to eq(1000)
+ expect(diff_options).to include(Commit.max_diff_options)
end
it 'should return safe limit for a diff if force diff is false' do
- expect(allowed_diff_size).to eq(100)
- end
- end
-
- describe 'allowed_diff_lines' do
- it 'should return hard limit for number of lines in a diff if force diff is true' do
- allow(controller).to receive(:params) { { force_show_diff: true } }
- expect(allowed_diff_lines).to eq(50000)
- end
-
- it 'should return safe limit for numbers of lines a diff if force diff is false' do
- expect(allowed_diff_lines).to eq(5000)
- end
- end
-
- describe 'safe_diff_files' do
- it 'should return all files from a commit that is smaller than safe limits' do
- expect(safe_diff_files(diffs, diff_refs).length).to eq(2)
- end
-
- it 'should return only the first file if the diff line count in the 2nd file takes the total beyond safe limits' do
- allow(diffs[1].diff).to receive(:lines).and_return([""] * 4999) #simulate 4999 lines
- expect(safe_diff_files(diffs, diff_refs).length).to eq(1)
- end
-
- it 'should return all files from a commit that is beyond safe limit for numbers of lines if force diff is true' do
- allow(controller).to receive(:params) { { force_show_diff: true } }
- allow(diffs[1].diff).to receive(:lines).and_return([""] * 4999) #simulate 4999 lines
- expect(safe_diff_files(diffs, diff_refs).length).to eq(2)
- end
-
- it 'should return only the first file if the diff line count in the 2nd file takes the total beyond hard limits' do
- allow(controller).to receive(:params) { { force_show_diff: true } }
- allow(diffs[1].diff).to receive(:lines).and_return([""] * 49999) #simulate 49999 lines
- expect(safe_diff_files(diffs, diff_refs).length).to eq(1)
- end
-
- it 'should return only a safe number of file diffs if a commit touches more files than the safe limits' do
- large_diffs = diffs * 100 #simulate 200 diffs
- expect(safe_diff_files(large_diffs, diff_refs).length).to eq(100)
- end
-
- it 'should return all file diffs if a commit touches more files than the safe limits but force diff is true' do
- allow(controller).to receive(:params) { { force_show_diff: true } }
- large_diffs = diffs * 100 #simulate 200 diffs
- expect(safe_diff_files(large_diffs, diff_refs).length).to eq(200)
- end
-
- it 'should return a limited file diffs if a commit touches more files than the hard limits and force diff is true' do
- allow(controller).to receive(:params) { { force_show_diff: true } }
- very_large_diffs = diffs * 1000 #simulate 2000 diffs
- expect(safe_diff_files(very_large_diffs, diff_refs).length).to eq(1000)
+ expect(diff_options).not_to include(:max_lines, :max_files)
end
end
diff --git a/spec/lib/gitlab/diff/parser_spec.rb b/spec/lib/gitlab/diff/parser_spec.rb
index fe0dea77909..f576c39284e 100644
--- a/spec/lib/gitlab/diff/parser_spec.rb
+++ b/spec/lib/gitlab/diff/parser_spec.rb
@@ -47,7 +47,7 @@ eos
end
before do
- @lines = parser.parse(diff.lines)
+ @lines = parser.parse(diff.lines).to_a
end
it { expect(@lines.size).to eq(30) }
diff --git a/spec/lib/gitlab/email/message/repository_push_spec.rb b/spec/lib/gitlab/email/message/repository_push_spec.rb
index 56ae2a8d121..b2d7a799810 100644
--- a/spec/lib/gitlab/email/message/repository_push_spec.rb
+++ b/spec/lib/gitlab/email/message/repository_push_spec.rb
@@ -72,7 +72,7 @@ describe Gitlab::Email::Message::RepositoryPush do
describe '#compare_timeout' do
subject { message.compare_timeout }
- it { is_expected.to eq compare.timeout }
+ it { is_expected.to eq compare.diffs.overflow? }
end
describe '#reverse_compare?' do
diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb
index 450250ba032..fea8182bd30 100644
--- a/spec/services/merge_requests/refresh_service_spec.rb
+++ b/spec/services/merge_requests/refresh_service_spec.rb
@@ -79,7 +79,7 @@ describe MergeRequests::RefreshService, services: true do
it { expect(@merge_request.notes.last.note).to include('changed to merged') }
it { expect(@merge_request).to be_merged }
- it { expect(@merge_request.diffs.length).to be > 0 }
+ it { expect(@merge_request.diffs.size).to be > 0 }
it { expect(@fork_merge_request).to be_merged }
it { expect(@fork_merge_request.notes.last.note).to include('changed to merged') }
end
--
GitLab