From c86c1905b5574cac234315598d8d715fcaee3ea7 Mon Sep 17 00:00:00 2001
From: Paco Guzman <pacoguzmanp@gmail.com>
Date: Wed, 27 Jul 2016 19:00:34 +0200
Subject: switch from diff_file_collection to diffs

So we have raw_diffs too
---
 app/controllers/projects/commit_controller.rb      |  4 +-
 app/controllers/projects/compare_controller.rb     |  4 +-
 .../projects/merge_requests_controller.rb          |  8 +--
 app/helpers/diff_helper.rb                         |  5 +-
 app/models/commit.rb                               | 10 ++-
 app/models/compare.rb                              | 36 +++++-----
 app/models/legacy_diff_note.rb                     |  4 +-
 app/models/merge_request.rb                        |  8 +--
 app/models/repository.rb                           |  2 +-
 app/services/compare_service.rb                    |  1 -
 .../merge_request_diff_cache_service.rb            |  2 +-
 app/views/projects/diffs/_diffs.html.haml          | 14 ++--
 .../projects/merge_requests/show/_diffs.html.haml  |  3 +-
 app/workers/emails_on_push_worker.rb               |  9 +--
 app/workers/irker_worker.rb                        |  6 +-
 lib/api/commits.rb                                 |  4 +-
 lib/api/entities.rb                                |  2 +-
 lib/gitlab/diff/file_collection.rb                 |  9 ---
 lib/gitlab/diff/file_collection/base.rb            | 17 +++--
 lib/gitlab/diff/file_collection/commit.rb          |  5 +-
 lib/gitlab/diff/file_collection/compare.rb         |  5 +-
 lib/gitlab/diff/file_collection/merge_request.rb   | 23 ++-----
 lib/gitlab/email/message/repository_push.rb        |  3 +-
 spec/helpers/diff_helper_spec.rb                   | 24 +++----
 spec/lib/gitlab/diff/file_spec.rb                  |  2 +-
 spec/lib/gitlab/diff/highlight_spec.rb             |  2 +-
 spec/lib/gitlab/diff/line_mapper_spec.rb           |  2 +-
 spec/lib/gitlab/diff/parallel_diff_spec.rb         |  2 +-
 spec/lib/gitlab/diff/parser_spec.rb                |  2 +-
 spec/models/compare_spec.rb                        | 77 ++++++++++++++++++++++
 spec/models/legacy_diff_note_spec.rb               |  2 +-
 spec/models/merge_request_spec.rb                  | 29 +++++++-
 spec/models/repository_spec.rb                     |  4 +-
 spec/requests/api/commits_spec.rb                  |  4 +-
 .../merge_request_diff_cache_service_spec.rb       |  4 +-
 35 files changed, 208 insertions(+), 130 deletions(-)
 delete mode 100644 lib/gitlab/diff/file_collection.rb
 create mode 100644 spec/models/compare_spec.rb

diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb
index 771a86530cd..fdfe7c65b7b 100644
--- a/app/controllers/projects/commit_controller.rb
+++ b/app/controllers/projects/commit_controller.rb
@@ -28,7 +28,7 @@ class Projects::CommitController < Projects::ApplicationController
   end
 
   def diff_for_path
-    render_diff_for_path(@commit.diff_file_collection(diff_options))
+    render_diff_for_path(@commit.diffs(diff_options))
   end
 
   def builds
@@ -110,7 +110,7 @@ class Projects::CommitController < Projects::ApplicationController
     opts = diff_options
     opts[:ignore_whitespace_change] = true if params[:format] == 'diff'
 
-    @diffs = commit.diff_file_collection(opts)
+    @diffs = commit.diffs(opts)
     @notes_count = commit.notes.count
   end
 
diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb
index 7fca5e77f32..4a42a7d091b 100644
--- a/app/controllers/projects/compare_controller.rb
+++ b/app/controllers/projects/compare_controller.rb
@@ -21,7 +21,7 @@ class Projects::CompareController < Projects::ApplicationController
   def diff_for_path
     return render_404 unless @compare
 
-    render_diff_for_path(@compare.diff_file_collection(diff_options: diff_options))
+    render_diff_for_path(@compare.diffs(diff_options: diff_options))
   end
 
   def create
@@ -45,7 +45,7 @@ class Projects::CompareController < Projects::ApplicationController
       @commit = @compare.commit
       @base_commit = @compare.base_commit
 
-      @diffs = @compare.diff_file_collection(diff_options: diff_options)
+      @diffs = @compare.diffs(diff_options: diff_options)
 
       @diff_notes_disabled = true
       @grouped_diff_discussions = {}
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 20afc6afcb2..2cf6a2dd1b3 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -86,7 +86,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     respond_to do |format|
       format.html { define_discussion_vars }
       format.json do
-        @diffs = @merge_request.diff_file_collection(diff_options)
+        @diffs = @merge_request.diffs(diff_options)
 
         render json: { html: view_to_html_string("projects/merge_requests/show/_diffs") }
       end
@@ -108,7 +108,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
     define_commit_vars
 
-    render_diff_for_path(@merge_request.diff_file_collection(diff_options))
+    render_diff_for_path(@merge_request.diffs(diff_options))
   end
 
   def commits
@@ -156,9 +156,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     @commits = @merge_request.compare_commits.reverse
     @commit = @merge_request.diff_head_commit
     @base_commit = @merge_request.diff_base_commit
-    if @merge_request.compare
-      @diffs = @merge_request.diff_file_collection(diff_options)
-    end
+    @diffs = @merge_request.diffs(diff_options) if @merge_request.compare
     @diff_notes_disabled = true
 
     @pipeline = @merge_request.pipeline
diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb
index 2abe24b78bf..cc7121b1163 100644
--- a/app/helpers/diff_helper.rb
+++ b/app/helpers/diff_helper.rb
@@ -23,10 +23,7 @@ module DiffHelper
   end
 
   def diff_options
-    options = Gitlab::Diff::FileCollection.default_options.merge(
-      ignore_whitespace_change: hide_whitespace?,
-      no_collapse: expand_all_diffs?
-    )
+    options = { ignore_whitespace_change: hide_whitespace?, no_collapse: expand_all_diffs? }
 
     if action_name == 'diff_for_path'
       options[:no_collapse] = true
diff --git a/app/models/commit.rb b/app/models/commit.rb
index a339d47f5f3..d58c2fb8106 100644
--- a/app/models/commit.rb
+++ b/app/models/commit.rb
@@ -104,7 +104,7 @@ class Commit
   end
 
   def diff_line_count
-    @diff_line_count ||= Commit::diff_line_count(self.diffs)
+    @diff_line_count ||= Commit::diff_line_count(raw_diffs)
     @diff_line_count
   end
 
@@ -317,7 +317,11 @@ class Commit
     nil
   end
 
-  def diff_file_collection(diff_options = nil)
+  def raw_diffs(*args)
+    raw.diffs(*args)
+  end
+
+  def diffs(diff_options = nil)
     Gitlab::Diff::FileCollection::Commit.new(self, diff_options: diff_options)
   end
 
@@ -330,7 +334,7 @@ class Commit
   def repo_changes
     changes = { added: [], modified: [], removed: [] }
 
-    diffs.each do |diff|
+    raw_diffs.each do |diff|
       if diff.deleted_file
         changes[:removed] << diff.old_path
       elsif diff.renamed_file || diff.new_file
diff --git a/app/models/compare.rb b/app/models/compare.rb
index 05c8fbbcc36..98c042f3809 100644
--- a/app/models/compare.rb
+++ b/app/models/compare.rb
@@ -1,6 +1,8 @@
 class Compare
   delegate :same, :head, :base, to: :@compare
 
+  attr_reader :project
+
   def self.decorate(compare, project)
     if compare.is_a?(Compare)
       compare
@@ -15,49 +17,47 @@ class Compare
   end
 
   def commits
-    @commits ||= Commit.decorate(@compare.commits, @project)
+    @commits ||= Commit.decorate(@compare.commits, project)
   end
 
   def start_commit
     return @start_commit if defined?(@start_commit)
 
     commit = @compare.base
-    @start_commit = commit ? ::Commit.new(commit, @project) : nil
+    @start_commit = commit ? ::Commit.new(commit, project) : nil
   end
 
-  def commit
-    return @commit if defined?(@commit)
+  def head_commit
+    return @head_commit if defined?(@head_commit)
 
     commit = @compare.head
-    @commit = commit ? ::Commit.new(commit, @project) : nil
-  end
-  alias_method :head_commit, :commit
-
-  # Used only on emails_on_push_worker.rb
-  def base_commit=(commit)
-    @base_commit = commit
+    @head_commit = commit ? ::Commit.new(commit, project) : nil
   end
+  alias_method :commit, :head_commit
 
   def base_commit
     return @base_commit if defined?(@base_commit)
 
-    @base_commit = if start_commit && commit
-                     @project.merge_base_commit(start_commit.id, commit.id)
+    @base_commit = if start_commit && head_commit
+                     project.merge_base_commit(start_commit.id, head_commit.id)
                    else
                      nil
                    end
   end
 
-  # keyword args until we get ride of diff_refs as argument
-  def diff_file_collection(diff_options:, diff_refs: self.diff_refs)
-    Gitlab::Diff::FileCollection::Compare.new(@compare,
-      project: @project,
+  def raw_diffs(*args)
+    @compare.diffs(*args)
+  end
+
+  def diffs(diff_options:)
+    Gitlab::Diff::FileCollection::Compare.new(self,
+      project: project,
       diff_options: diff_options,
       diff_refs: diff_refs)
   end
 
   def diff_refs
-    @diff_refs ||= Gitlab::Diff::DiffRefs.new(
+    Gitlab::Diff::DiffRefs.new(
       base_sha:  base_commit.try(:sha),
       start_sha: start_commit.try(:sha),
       head_sha: commit.try(:sha)
diff --git a/app/models/legacy_diff_note.rb b/app/models/legacy_diff_note.rb
index 865712268a0..6ed66001513 100644
--- a/app/models/legacy_diff_note.rb
+++ b/app/models/legacy_diff_note.rb
@@ -85,7 +85,7 @@ class LegacyDiffNote < Note
     return nil unless noteable
     return @diff if defined?(@diff)
 
-    @diff = noteable.diffs(Commit.max_diff_options).find do |d|
+    @diff = noteable.raw_diffs(Commit.max_diff_options).find do |d|
       d.new_path && Digest::SHA1.hexdigest(d.new_path) == diff_file_hash
     end
   end
@@ -116,7 +116,7 @@ class LegacyDiffNote < Note
 
   # Find the diff on noteable that matches our own
   def find_noteable_diff
-    diffs = noteable.diffs(Commit.max_diff_options)
+    diffs = noteable.raw_diffs(Commit.max_diff_options)
     diffs.find { |d| d.new_path == self.diff.new_path }
   end
 end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 62e5573dfdc..009262d6b48 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -164,13 +164,13 @@ class MergeRequest < ActiveRecord::Base
     merge_request_diff ? merge_request_diff.first_commit : compare_commits.first
   end
 
-  def diffs(*args)
-    merge_request_diff ? merge_request_diff.diffs(*args) : compare.diffs(*args)
+  def raw_diffs(*args)
+    merge_request_diff ? merge_request_diff.diffs(*args) : compare.raw_diffs(*args)
   end
 
-  def diff_file_collection(diff_options = nil)
+  def diffs(diff_options = nil)
     if self.compare
-      self.compare.diff_file_collection(diff_options: diff_options, diff_refs: diff_refs)
+      self.compare.diffs(diff_options: diff_options)
     else
       Gitlab::Diff::FileCollection::MergeRequest.new(self, diff_options: diff_options)
     end
diff --git a/app/models/repository.rb b/app/models/repository.rb
index bac37483c47..3d95344a68f 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -372,7 +372,7 @@ class Repository
     # We don't want to flush the cache if the commit didn't actually make any
     # changes to any of the possible avatar files.
     if revision && commit = self.commit(revision)
-      return unless commit.diffs.
+      return unless commit.raw_diffs.
         any? { |diff| AVATAR_FILES.include?(diff.new_path) }
     end
 
diff --git a/app/services/compare_service.rb b/app/services/compare_service.rb
index bb3aff72b47..6d6075628af 100644
--- a/app/services/compare_service.rb
+++ b/app/services/compare_service.rb
@@ -26,7 +26,6 @@ class CompareService
       source_sha
     )
 
-    # REVIEW be sure if it's target_project or source_project
     Compare.new(raw_compare, target_project)
   end
 end
diff --git a/app/services/merge_requests/merge_request_diff_cache_service.rb b/app/services/merge_requests/merge_request_diff_cache_service.rb
index 8151c24d1b0..2945a7fd4e4 100644
--- a/app/services/merge_requests/merge_request_diff_cache_service.rb
+++ b/app/services/merge_requests/merge_request_diff_cache_service.rb
@@ -2,7 +2,7 @@ module MergeRequests
   class MergeRequestDiffCacheService
     def execute(merge_request)
       # Executing the iteration we cache all the highlighted diff information
-      merge_request.diff_file_collection.diff_files.to_a
+      merge_request.diffs.diff_files.to_a
     end
   end
 end
diff --git a/app/views/projects/diffs/_diffs.html.haml b/app/views/projects/diffs/_diffs.html.haml
index 35fdf7d5278..20dc280c3b2 100644
--- a/app/views/projects/diffs/_diffs.html.haml
+++ b/app/views/projects/diffs/_diffs.html.haml
@@ -9,11 +9,11 @@
       = link_to 'Expand all', url_for(params.merge(expand_all_diffs: 1, format: nil)), class: 'btn btn-default'
     - if show_whitespace_toggle
       - if current_controller?(:commit)
-        = commit_diff_whitespace_link(@project, @commit, class: 'hidden-xs')
+        = commit_diff_whitespace_link(diffs.project, @commit, class: 'hidden-xs')
       - elsif current_controller?(:merge_requests)
-        = diff_merge_request_whitespace_link(@project, @merge_request, class: 'hidden-xs')
+        = diff_merge_request_whitespace_link(diffs.project, @merge_request, class: 'hidden-xs')
       - elsif current_controller?(:compare)
-        = diff_compare_whitespace_link(@project, params[:from], params[:to], class: 'hidden-xs')
+        = diff_compare_whitespace_link(diffs.project, params[:from], params[:to], class: 'hidden-xs')
     .btn-group
       = inline_diff_btn
       = parallel_diff_btn
@@ -22,12 +22,12 @@
 - if diff_files.overflow?
   = render 'projects/diffs/warning', diff_files: diff_files
 
-.files{data: {can_create_note: (!@diff_notes_disabled && can?(current_user, :create_note, @project))}}
+.files{data: {can_create_note: (!@diff_notes_disabled && can?(current_user, :create_note, diffs.project))}}
   - diff_files.each_with_index do |diff_file, index|
     - diff_commit = commit_for_diff(diff_file)
     - blob = diff_file.blob(diff_commit)
     - next unless blob
-    - blob.load_all_data!(@project.repository) unless blob.only_display_raw?
+    - blob.load_all_data!(diffs.project.repository) unless blob.only_display_raw?
 
-    = render 'projects/diffs/file', i: index, project: @project,
-      diff_file: diff_file, diff_commit: diff_commit, blob: blob, diff_refs: diffs.diff_refs
+    = render 'projects/diffs/file', i: index, project: diffs.project,
+      diff_file: diff_file, diff_commit: diff_commit, blob: blob
diff --git a/app/views/projects/merge_requests/show/_diffs.html.haml b/app/views/projects/merge_requests/show/_diffs.html.haml
index c6d2567af35..013b05628fa 100644
--- a/app/views/projects/merge_requests/show/_diffs.html.haml
+++ b/app/views/projects/merge_requests/show/_diffs.html.haml
@@ -1,6 +1,5 @@
 - if @merge_request_diff.collected?
-  = render "projects/diffs/diffs", diffs: @diffs,
-    diff_refs: @diffs.diff_refs, project: @diffs.project
+  = render "projects/diffs/diffs", diffs: @diffs
 - elsif @merge_request_diff.empty?
   .nothing-here-block Nothing to merge from #{@merge_request.source_branch} into #{@merge_request.target_branch}
 - else
diff --git a/app/workers/emails_on_push_worker.rb b/app/workers/emails_on_push_worker.rb
index 0b63913cfd1..c6a5af2809a 100644
--- a/app/workers/emails_on_push_worker.rb
+++ b/app/workers/emails_on_push_worker.rb
@@ -33,18 +33,13 @@ class EmailsOnPushWorker
     reverse_compare = false
 
     if action == :push
-      base_commit = project.merge_base_commit(before_sha, after_sha)
-      compare = Gitlab::Git::Compare.new(project.repository.raw_repository, before_sha, after_sha)
-      compare = Compare.decorate(compare, project)
-      compare.base_commit = base_commit
+      compare = CompareService.new.execute(project, before_sha, project, after_sha)
       diff_refs = compare.diff_refs
 
       return false if compare.same
 
       if compare.commits.empty?
-        compare = Gitlab::Git::Compare.new(project.repository.raw_repository, after_sha, before_sha)
-        compare = Compare.decorate(compare, project)
-        compare.base_commit = base_commit
+        compare = CompareService.new.execute(project, after_sha, project, before_sha)
         diff_refs = compare.diff_refs
 
         reverse_compare = true
diff --git a/app/workers/irker_worker.rb b/app/workers/irker_worker.rb
index a3c34e02baa..07cc7c1cbd7 100644
--- a/app/workers/irker_worker.rb
+++ b/app/workers/irker_worker.rb
@@ -141,8 +141,10 @@ class IrkerWorker
   end
 
   def files_count(commit)
-    files = "#{commit.diffs.real_size} file"
-    files += 's' if commit.diffs.size > 1
+    diffs = commit.raw_diffs
+
+    files = "#{diffs.real_size} file"
+    files += 's' if diffs.size > 1
     files
   end
 
diff --git a/lib/api/commits.rb b/lib/api/commits.rb
index 4a11c8e3620..b4eaf1813d4 100644
--- a/lib/api/commits.rb
+++ b/lib/api/commits.rb
@@ -54,7 +54,7 @@ module API
         sha = params[:sha]
         commit = user_project.commit(sha)
         not_found! "Commit" unless commit
-        commit.diffs.to_a
+        commit.raw_diffs.to_a
       end
 
       # Get a commit's comments
@@ -96,7 +96,7 @@ module API
         }
 
         if params[:path] && params[:line] && params[:line_type]
-          commit.diffs(all_diffs: true).each do |diff|
+          commit.raw_diffs(all_diffs: true).each do |diff|
             next unless diff.new_path == params[:path]
             lines = Gitlab::Diff::Parser.new.parse(diff.diff.each_line)
 
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index 3e21b7a0b8a..e5b00dc45a5 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -224,7 +224,7 @@ module API
 
     class MergeRequestChanges < MergeRequest
       expose :diffs, as: :changes, using: Entities::RepoDiff do |compare, _|
-        compare.diffs(all_diffs: true).to_a
+        compare.raw_diffs(all_diffs: true).to_a
       end
     end
 
diff --git a/lib/gitlab/diff/file_collection.rb b/lib/gitlab/diff/file_collection.rb
deleted file mode 100644
index ce6717c7205..00000000000
--- a/lib/gitlab/diff/file_collection.rb
+++ /dev/null
@@ -1,9 +0,0 @@
-module Gitlab
-  module Diff
-    module FileCollection
-      def self.default_options
-        ::Commit.max_diff_options.merge(ignore_whitespace_change: false, no_collapse: false)
-      end
-    end
-  end
-end
diff --git a/lib/gitlab/diff/file_collection/base.rb b/lib/gitlab/diff/file_collection/base.rb
index a0c88265c45..2b9fc65b985 100644
--- a/lib/gitlab/diff/file_collection/base.rb
+++ b/lib/gitlab/diff/file_collection/base.rb
@@ -1,28 +1,33 @@
 module Gitlab
   module Diff
     module FileCollection
-
       class Base
         attr_reader :project, :diff_options, :diff_view, :diff_refs
 
         delegate :count, :size, :real_size, to: :diff_files
 
-        def initialize(diffs, project:, diff_options: nil, diff_refs: nil)
-          @diffs        = diffs
+        def self.default_options
+          ::Commit.max_diff_options.merge(ignore_whitespace_change: false, no_collapse: false)
+        end
+
+        def initialize(diffable, project:, diff_options: nil, diff_refs: nil)
+          diff_options = self.class.default_options.merge(diff_options || {})
+
+          @diffable     = diffable
+          @diffs        = diffable.raw_diffs(diff_options)
           @project      = project
           @diff_options = diff_options
           @diff_refs    = diff_refs
         end
 
         def diff_files
-          @diffs.decorate! { |diff| decorate_diff!(diff) }
+          @diff_files ||= @diffs.decorate! { |diff| decorate_diff!(diff) }
         end
 
         private
 
         def decorate_diff!(diff)
-          return diff if diff.is_a?(Gitlab::Diff::File)
-          Gitlab::Diff::File.new(diff, diff_refs: @diff_refs, repository: @project.repository)
+          Gitlab::Diff::File.new(diff, repository: project.repository, diff_refs: diff_refs)
         end
       end
     end
diff --git a/lib/gitlab/diff/file_collection/commit.rb b/lib/gitlab/diff/file_collection/commit.rb
index 19def300b74..4dc297ec036 100644
--- a/lib/gitlab/diff/file_collection/commit.rb
+++ b/lib/gitlab/diff/file_collection/commit.rb
@@ -3,10 +3,7 @@ module Gitlab
     module FileCollection
       class Commit < Base
         def initialize(commit, diff_options:)
-          # Not merge just set defaults
-          diff_options = diff_options || Gitlab::Diff::FileCollection.default_options
-
-          super(commit.diffs(diff_options),
+          super(commit,
             project: commit.project,
             diff_options: diff_options,
             diff_refs: commit.diff_refs)
diff --git a/lib/gitlab/diff/file_collection/compare.rb b/lib/gitlab/diff/file_collection/compare.rb
index aba5a28b51f..20d8f891cc3 100644
--- a/lib/gitlab/diff/file_collection/compare.rb
+++ b/lib/gitlab/diff/file_collection/compare.rb
@@ -3,10 +3,7 @@ module Gitlab
     module FileCollection
       class Compare < Base
         def initialize(compare, project:, diff_options:, diff_refs: nil)
-          # Not merge just set defaults
-          diff_options = diff_options || Gitlab::Diff::FileCollection.default_options
-
-          super(compare.diffs(diff_options),
+          super(compare,
             project:      project,
             diff_options: diff_options,
             diff_refs:    diff_refs)
diff --git a/lib/gitlab/diff/file_collection/merge_request.rb b/lib/gitlab/diff/file_collection/merge_request.rb
index 9fde0bba183..4f946908e2f 100644
--- a/lib/gitlab/diff/file_collection/merge_request.rb
+++ b/lib/gitlab/diff/file_collection/merge_request.rb
@@ -4,10 +4,8 @@ module Gitlab
       class MergeRequest < Base
         def initialize(merge_request, diff_options:)
           @merge_request = merge_request
-          # Not merge just set defaults
-          diff_options = diff_options || Gitlab::Diff::FileCollection.default_options
 
-          super(merge_request.diffs(diff_options),
+          super(merge_request,
             project: merge_request.project,
             diff_options: diff_options,
             diff_refs: merge_request.diff_refs)
@@ -19,18 +17,11 @@ module Gitlab
 
         private
 
-        # Extracted method to highlight in the same iteration to the diff_collection. Iteration in the DiffCollections
-        # seems particularly slow on big diffs (event when already populated).
+        # Extracted method to highlight in the same iteration to the diff_collection.
         def decorate_diff!(diff)
-          highlight! super
-        end
-
-        def highlight!(diff_file)
-          if cacheable?
-            cache_highlight!(diff_file)
-          else
-            diff_file # Don't need to eager load highlighted diff lines
-          end
+          diff_file = super
+          cache_highlight!(diff_file) if cacheable?
+          diff_file
         end
 
         def highlight_diff_file_from_cache!(diff_file, cache_diff_lines)
@@ -55,15 +46,13 @@ module Gitlab
           else
             highlight_cache[file_path] = diff_file.highlighted_diff_lines.map(&:to_hash)
           end
-
-          diff_file
         end
 
         def highlight_cache
           return @highlight_cache if defined?(@highlight_cache)
 
           @highlight_cache = Rails.cache.read(cache_key) || {}
-          @highlight_cache_was_empty = highlight_cache.empty?
+          @highlight_cache_was_empty = @highlight_cache.empty?
           @highlight_cache
         end
 
diff --git a/lib/gitlab/email/message/repository_push.rb b/lib/gitlab/email/message/repository_push.rb
index 16491ede71b..62d29387d60 100644
--- a/lib/gitlab/email/message/repository_push.rb
+++ b/lib/gitlab/email/message/repository_push.rb
@@ -41,7 +41,8 @@ module Gitlab
         def diffs
           return unless compare
 
-          @diffs ||= compare.diff_file_collection(diff_options: { max_files: 30 }).diff_files
+          # This diff is more moderated in number of files and lines
+          @diffs ||= compare.diffs(diff_options: { max_files: 30, max_lines: 5000, no_collapse: true }).diff_files
         end
 
         def diffs_count
diff --git a/spec/helpers/diff_helper_spec.rb b/spec/helpers/diff_helper_spec.rb
index c2fd2c8a533..4949280d641 100644
--- a/spec/helpers/diff_helper_spec.rb
+++ b/spec/helpers/diff_helper_spec.rb
@@ -6,7 +6,7 @@ describe DiffHelper do
   let(:project) { create(:project) }
   let(:repository) { project.repository }
   let(:commit) { project.commit(sample_commit.id) }
-  let(:diffs) { commit.diffs }
+  let(:diffs) { commit.raw_diffs }
   let(:diff) { diffs.first }
   let(:diff_refs) { [commit.parent, commit] }
   let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: diff_refs, repository: repository) }
@@ -32,16 +32,6 @@ describe DiffHelper do
   end
   
   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(diff_options).to include(Commit.max_diff_options)
-    end
-
-    it 'should return hard limit for a diff if expand_all_diffs is true' do
-      allow(controller).to receive(:params) { { expand_all_diffs: true } }
-      expect(diff_options).to include(Commit.max_diff_options)
-    end
-
     it 'should return no collapse false' do
       expect(diff_options).to include(no_collapse: false)
     end
@@ -55,6 +45,18 @@ describe DiffHelper do
       allow(controller).to receive(:action_name) { 'diff_for_path' }
       expect(diff_options).to include(no_collapse: true)
     end
+
+    it 'should return paths if action name diff_for_path and param old path' do
+      allow(controller).to receive(:params) { { old_path: 'lib/wadus.rb' } }
+      allow(controller).to receive(:action_name) { 'diff_for_path' }
+      expect(diff_options[:paths]).to include('lib/wadus.rb')
+    end
+
+    it 'should return paths if action name diff_for_path and param new path' do
+      allow(controller).to receive(:params) { { new_path: 'lib/wadus.rb' } }
+      allow(controller).to receive(:action_name) { 'diff_for_path' }
+      expect(diff_options[:paths]).to include('lib/wadus.rb')
+    end
   end
 
   describe 'unfold_bottom_class' do
diff --git a/spec/lib/gitlab/diff/file_spec.rb b/spec/lib/gitlab/diff/file_spec.rb
index e883a6eb9c2..0650cb291e5 100644
--- a/spec/lib/gitlab/diff/file_spec.rb
+++ b/spec/lib/gitlab/diff/file_spec.rb
@@ -5,7 +5,7 @@ describe Gitlab::Diff::File, lib: true do
 
   let(:project) { create(:project) }
   let(:commit) { project.commit(sample_commit.id) }
-  let(:diff) { commit.diffs.first }
+  let(:diff) { commit.raw_diffs.first }
   let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: commit.diff_refs, repository: project.repository) }
 
   describe '#diff_lines' do
diff --git a/spec/lib/gitlab/diff/highlight_spec.rb b/spec/lib/gitlab/diff/highlight_spec.rb
index 88e4115c453..1c2ddeed692 100644
--- a/spec/lib/gitlab/diff/highlight_spec.rb
+++ b/spec/lib/gitlab/diff/highlight_spec.rb
@@ -5,7 +5,7 @@ describe Gitlab::Diff::Highlight, lib: true do
 
   let(:project) { create(:project) }
   let(:commit) { project.commit(sample_commit.id) }
-  let(:diff) { commit.diffs.first }
+  let(:diff) { commit.raw_diffs.first }
   let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: commit.diff_refs, repository: project.repository) }
 
   describe '#highlight' do
diff --git a/spec/lib/gitlab/diff/line_mapper_spec.rb b/spec/lib/gitlab/diff/line_mapper_spec.rb
index 4e50e03bb7e..4b943fa382d 100644
--- a/spec/lib/gitlab/diff/line_mapper_spec.rb
+++ b/spec/lib/gitlab/diff/line_mapper_spec.rb
@@ -6,7 +6,7 @@ describe Gitlab::Diff::LineMapper, lib: true do
   let(:project) { create(:project) }
   let(:repository) { project.repository }
   let(:commit) { project.commit(sample_commit.id) }
-  let(:diffs) { commit.diffs }
+  let(:diffs) { commit.raw_diffs }
   let(:diff) { diffs.first }
   let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: commit.diff_refs, repository: repository) }
   subject { described_class.new(diff_file) }
diff --git a/spec/lib/gitlab/diff/parallel_diff_spec.rb b/spec/lib/gitlab/diff/parallel_diff_spec.rb
index 2aa5ae44f54..af18d3c25a6 100644
--- a/spec/lib/gitlab/diff/parallel_diff_spec.rb
+++ b/spec/lib/gitlab/diff/parallel_diff_spec.rb
@@ -6,7 +6,7 @@ describe Gitlab::Diff::ParallelDiff, lib: true do
   let(:project) { create(:project) }
   let(:repository) { project.repository }
   let(:commit) { project.commit(sample_commit.id) }
-  let(:diffs) { commit.diffs }
+  let(:diffs) { commit.raw_diffs }
   let(:diff) { diffs.first }
   let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: commit.diff_refs, repository: repository) }
   subject { described_class.new(diff_file) }
diff --git a/spec/lib/gitlab/diff/parser_spec.rb b/spec/lib/gitlab/diff/parser_spec.rb
index c3359627652..b983d73f8be 100644
--- a/spec/lib/gitlab/diff/parser_spec.rb
+++ b/spec/lib/gitlab/diff/parser_spec.rb
@@ -5,7 +5,7 @@ describe Gitlab::Diff::Parser, lib: true do
 
   let(:project) { create(:project) }
   let(:commit) { project.commit(sample_commit.id) }
-  let(:diff) { commit.diffs.first }
+  let(:diff) { commit.raw_diffs.first }
   let(:parser) { Gitlab::Diff::Parser.new }
 
   describe '#parse' do
diff --git a/spec/models/compare_spec.rb b/spec/models/compare_spec.rb
new file mode 100644
index 00000000000..49ab3c4b6e9
--- /dev/null
+++ b/spec/models/compare_spec.rb
@@ -0,0 +1,77 @@
+require 'spec_helper'
+
+describe Compare, models: true do
+  include RepoHelpers
+
+  let(:project) { create(:project, :public) }
+  let(:commit)  { project.commit }
+
+  let(:start_commit) { sample_image_commit }
+  let(:head_commit) { sample_commit }
+
+  let(:raw_compare) { Gitlab::Git::Compare.new(project.repository.raw_repository, start_commit.id, head_commit.id) }
+
+  subject { described_class.new(raw_compare, project) }
+
+  describe '#start_commit' do
+    it 'returns raw compare base commit' do
+      expect(subject.start_commit.id).to eq(start_commit.id)
+    end
+
+    it 'returns nil if compare base commit is nil' do
+      expect(raw_compare).to receive(:base).and_return(nil)
+
+      expect(subject.start_commit).to eq(nil)
+    end
+  end
+
+  describe '#commit' do
+    it 'returns raw compare head commit' do
+      expect(subject.commit.id).to eq(head_commit.id)
+    end
+
+    it 'returns nil if compare head commit is nil' do
+      expect(raw_compare).to receive(:head).and_return(nil)
+
+      expect(subject.commit).to eq(nil)
+    end
+  end
+
+  describe '#base_commit' do
+    let(:base_commit) { Commit.new(another_sample_commit, project) }
+
+    it 'returns project merge base commit' do
+      expect(project).to receive(:merge_base_commit).with(start_commit.id, head_commit.id).and_return(base_commit)
+
+      expect(subject.base_commit).to eq(base_commit)
+    end
+
+    it 'returns nil if there is no start_commit' do
+      expect(subject).to receive(:start_commit).and_return(nil)
+
+      expect(subject.base_commit).to eq(nil)
+    end
+
+    it 'returns nil if there is no head commit' do
+      expect(subject).to receive(:head_commit).and_return(nil)
+
+      expect(subject.base_commit).to eq(nil)
+    end
+  end
+
+  describe '#diff_refs' do
+    it 'uses base_commit sha as base_sha' do
+      expect(subject).to receive(:base_commit).at_least(:once).and_call_original
+
+      expect(subject.diff_refs.base_sha).to eq(subject.base_commit.id)
+    end
+
+    it 'uses start_commit sha as start_sha' do
+      expect(subject.diff_refs.start_sha).to eq(start_commit.id)
+    end
+
+    it 'uses commit sha as head sha' do
+      expect(subject.diff_refs.head_sha).to eq(head_commit.id)
+    end
+  end
+end
diff --git a/spec/models/legacy_diff_note_spec.rb b/spec/models/legacy_diff_note_spec.rb
index d23fc06c3ad..c8ee656fe3b 100644
--- a/spec/models/legacy_diff_note_spec.rb
+++ b/spec/models/legacy_diff_note_spec.rb
@@ -58,7 +58,7 @@ describe LegacyDiffNote, models: true do
 
         # Generate a real line_code value so we know it will match. We use a
         # random line from a random diff just for funsies.
-        diff = merge.diffs.to_a.sample
+        diff = merge.raw_diffs.to_a.sample
         line = Gitlab::Diff::Parser.new.parse(diff.diff.each_line).to_a.sample
         code = Gitlab::Diff::LineCode.generate(diff.new_path, line.new_pos, line.old_pos)
 
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index fa1f7edae8e..152e0cce5ad 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -128,7 +128,7 @@ describe MergeRequest, models: true do
     end
   end
 
-  describe '#diffs' do
+  describe '#raw_diffs' do
     let(:merge_request) { build(:merge_request) }
     let(:options) { { paths: ['a/b', 'b/a', 'c/*'] } }
 
@@ -138,6 +138,31 @@ describe MergeRequest, models: true do
 
         expect(merge_request.merge_request_diff).to receive(:diffs).with(options)
 
+        merge_request.raw_diffs(options)
+      end
+    end
+
+    context 'when there are no MR diffs' do
+      it 'delegates to the compare object' do
+        merge_request.compare = double(:compare)
+
+        expect(merge_request.compare).to receive(:raw_diffs).with(options)
+
+        merge_request.raw_diffs(options)
+      end
+    end
+  end
+
+  describe '#diffs' do
+    let(:merge_request) { build(:merge_request) }
+    let(:options) { { paths: ['a/b', 'b/a', 'c/*'] } }
+
+    context 'when there are MR diffs' do
+      it 'delegates to the MR diffs' do
+        merge_request.merge_request_diff = MergeRequestDiff.new
+
+        expect(merge_request.merge_request_diff).to receive(:diffs).with(hash_including(options))
+
         merge_request.diffs(options)
       end
     end
@@ -146,7 +171,7 @@ describe MergeRequest, models: true do
       it 'delegates to the compare object' do
         merge_request.compare = double(:compare)
 
-        expect(merge_request.compare).to receive(:diffs).with(options)
+        expect(merge_request.compare).to receive(:diffs).with(diff_options: options)
 
         merge_request.diffs(options)
       end
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index cce15538b93..2a053b1804f 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -1154,7 +1154,7 @@ describe Repository, models: true do
       it 'does not flush the cache if the commit does not change any logos' do
         diff = double(:diff, new_path: 'test.txt')
 
-        expect(commit).to receive(:diffs).and_return([diff])
+        expect(commit).to receive(:raw_diffs).and_return([diff])
         expect(cache).not_to receive(:expire)
 
         repository.expire_avatar_cache(repository.root_ref, '123')
@@ -1163,7 +1163,7 @@ describe Repository, models: true do
       it 'flushes the cache if the commit changes any of the logos' do
         diff = double(:diff, new_path: Repository::AVATAR_FILES[0])
 
-        expect(commit).to receive(:diffs).and_return([diff])
+        expect(commit).to receive(:raw_diffs).and_return([diff])
         expect(cache).to receive(:expire).with(:avatar)
 
         repository.expire_avatar_cache(repository.root_ref, '123')
diff --git a/spec/requests/api/commits_spec.rb b/spec/requests/api/commits_spec.rb
index e4ea8506598..51ee2167d47 100644
--- a/spec/requests/api/commits_spec.rb
+++ b/spec/requests/api/commits_spec.rb
@@ -173,10 +173,10 @@ describe API::API, api: true  do
       end
 
       it 'should return the inline comment' do
-        post api("/projects/#{project.id}/repository/commits/#{project.repository.commit.id}/comments", user), note: 'My comment', path: project.repository.commit.diffs.first.new_path, line: 7, line_type: 'new'
+        post api("/projects/#{project.id}/repository/commits/#{project.repository.commit.id}/comments", user), note: 'My comment', path: project.repository.commit.raw_diffs.first.new_path, line: 7, line_type: 'new'
         expect(response).to have_http_status(201)
         expect(json_response['note']).to eq('My comment')
-        expect(json_response['path']).to eq(project.repository.commit.diffs.first.new_path)
+        expect(json_response['path']).to eq(project.repository.commit.raw_diffs.first.new_path)
         expect(json_response['line']).to eq(7)
         expect(json_response['line_type']).to eq('new')
       end
diff --git a/spec/services/merge_requests/merge_request_diff_cache_service_spec.rb b/spec/services/merge_requests/merge_request_diff_cache_service_spec.rb
index c6cceed31ad..8f71d71b0f0 100644
--- a/spec/services/merge_requests/merge_request_diff_cache_service_spec.rb
+++ b/spec/services/merge_requests/merge_request_diff_cache_service_spec.rb
@@ -5,9 +5,9 @@ describe MergeRequests::MergeRequestDiffCacheService do
   let(:subject) { MergeRequests::MergeRequestDiffCacheService.new }
 
   describe '#execute' do
-    it 'retrieve the diff files to cache the highlighted result' do
+    it 'retrieves the diff files to cache the highlighted result' do
       merge_request = create(:merge_request)
-      cache_key = [merge_request.merge_request_diff, 'highlighted-diff-files', Gitlab::Diff::FileCollection.default_options]
+      cache_key = [merge_request.merge_request_diff, 'highlighted-diff-files', Gitlab::Diff::FileCollection::MergeRequest.default_options]
 
       expect(Rails.cache).to receive(:read).with(cache_key).and_return({})
       expect(Rails.cache).to receive(:write).with(cache_key, anything)
-- 
cgit v1.2.1