diff options
36 files changed, 192 insertions, 294 deletions
@@ -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 |