Use Gitlab::Git::DiffCollections

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

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)

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

app/controllers/projects/compare_controller.rb

 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

app/controllers/projects/merge_requests_controller.rb

 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

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)

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

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

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
-    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)
+      diff_collection = unmerged_diffs
+      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
-    if new_diffs.present?
-      new_diffs = dump_commits(new_diffs)
-      self.state = :collected
+      self.real_size = diff_collection.real_size
+      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

app/models/note.rb

@@ -131,9 +131,11 @@ class Note < ActiveRecord::Base
   end
   def find_diff
-    return nil unless noteable && noteable.diffs.present?
-    @diff ||= noteable.diffs.find do |d|
+    return nil unless noteable
+    return @diff if defined?(@diff)
+    # 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
-    noteable.diffs.each do |mr_diff|
-      next unless mr_diff.new_path == self.diff.new_path
-      lines = Gitlab::Diff::Parser.new.parse(mr_diff.diff.lines.to_a)
-      lines.each do |line|
-        if line.text == diff_line
-          return true
-        end
-      end
-    end
-
-    false
+    return @active if defined?(@active)
+    diffs = noteable.diffs(Commit.max_diff_options)
+    notable_diff = diffs.find { |d| d.new_path == self.diff.new_path }
+    return @active = false if notable_diff.nil?
+    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

app/services/compare_service.rb

 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

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

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

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

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.

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

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"

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

app/views/projects/merge_requests/show/_diffs.html.haml

 - 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}

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