Use SafeDiffs everywhere20034-phase-2

20 files changed, 134 insertions, 47 deletions

diff --git a/app/controllers/concerns/diff_for_path.rb b/app/controllers/concerns/diff_for_path.rb
index 026d8b2e1e0..aeec3009f15 100644
--- a/app/controllers/concerns/diff_for_path.rb
+++ b/app/controllers/concerns/diff_for_path.rb
@@ -1,8 +1,8 @@
 module DiffForPath
   extend ActiveSupport::Concern
 
-  def render_diff_for_path(diffs, diff_refs, project)
-    diff_file = safe_diff_files(diffs, diff_refs: diff_refs, repository: project.repository).find do |diff|
+  def render_diff_for_path(diffs)
+    diff_file = diffs.diff_files.find do |diff|
       diff.old_path == params[:old_path] && diff.new_path == params[:new_path]
     end
 
@@ -14,7 +14,7 @@ module DiffForPath
     locals = {
       diff_file: diff_file,
       diff_commit: diff_commit,
-      diff_refs: diff_refs,
+      diff_refs: diffs.diff_refs,
       blob: blob,
       project: project
     }
diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb
index 727e84b40a1..d226e3dc40c 100644
--- a/app/controllers/projects/commit_controller.rb
+++ b/app/controllers/projects/commit_controller.rb
@@ -28,10 +28,11 @@ class Projects::CommitController < Projects::ApplicationController
   end
 
   def diff_for_path
-    render_diff_for_path(@diffs, @commit.diff_refs, @project)
+    render_diff_for_path(SafeDiffs::Commit.new(@commit, diff_options: diff_options))
   end
 
   def builds
+    # TODO only needs diffs.count from diffs
   end
 
   def cancel_builds
@@ -110,7 +111,7 @@ class Projects::CommitController < Projects::ApplicationController
     opts = diff_options
     opts[:ignore_whitespace_change] = true if params[:format] == 'diff'
 
-    @diffs = commit.diffs(opts)
+    @diffs = SafeDiffs::Commit.new(commit, diff_options: opts)
     @notes_count = commit.notes.count
   end
 
diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb
index 10749d0fef8..1a7d3fa6894 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(@diffs, @diff_refs, @project)
+    render_diff_for_path(SafeDiffs::Compare.new(@compare, project: @project, diff_options: diff_options))
   end
 
   def create
@@ -46,12 +46,12 @@ class Projects::CompareController < Projects::ApplicationController
       @commit = @project.commit(@head_ref)
       @base_commit = @project.merge_base_commit(@start_ref, @head_ref)
 
-      @diffs = @compare.diffs(diff_options)
-      @diff_refs = Gitlab::Diff::DiffRefs.new(
+      diff_refs = Gitlab::Diff::DiffRefs.new(
         base_sha: @base_commit.try(:sha),
         start_sha: @start_commit.try(:sha),
         head_sha: @commit.try(:sha)
       )
+      @diffs = SafeDiffs::Compare.new(@compare, project: @project, diff_options: diff_options, diff_refs: diff_refs)
 
       @diff_notes_disabled = true
       @grouped_diff_notes = {}
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 7beeb7d97d0..9ab268fd6e2 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -101,9 +101,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     end
 
     define_commit_vars
-    diffs = @merge_request.diffs(diff_options)
 
-    render_diff_for_path(diffs, @merge_request.diff_refs, @merge_request.project)
+    render_diff_for_path(SafeDiffs::MergeRequest.new(merge_request, diff_options: diff_options))
   end
 
   def commits
@@ -151,7 +150,12 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     @commits = @merge_request.compare_commits.reverse
     @commit = @merge_request.diff_head_commit
     @base_commit = @merge_request.diff_base_commit
-    @diffs = @merge_request.compare.diffs(diff_options) if @merge_request.compare
+    if @merge_request.compare
+      @diffs = SafeDiffs::Compare.new(@merge_request.compare,
+        project:      @merge_request.project,
+        diff_refs:    @merge_request.diff_refs,
+        diff_options: diff_options)
+    end
     @diff_notes_disabled = true
 
     @pipeline = @merge_request.pipeline
diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb
index 75b029365f9..11d46e69422 100644
--- a/app/helpers/diff_helper.rb
+++ b/app/helpers/diff_helper.rb
@@ -33,10 +33,6 @@ module DiffHelper
     Commit.max_diff_options.merge(options)
   end
 
-  def safe_diff_files(diffs, diff_refs: nil, repository: nil)
-    diffs.decorate! { |diff| Gitlab::Diff::File.new(diff, diff_refs: diff_refs, repository: repository) }
-  end
-
   def unfold_bottom_class(bottom)
     bottom ? 'js-unfold js-unfold-bottom' : ''
   end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index eaa9ab48cf9..df877e9eb46 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -165,6 +165,7 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def safe_diff_files(*args)
+    # We always do this except when count
     diffs(*args).decorate! do |diff|
       Gitlab::Diff::File.new(diff, diff_refs: diff_refs, repository: project.repository)
     end
diff --git a/app/models/safe_diffs/base.rb b/app/models/safe_diffs/base.rb
new file mode 100644
index 00000000000..c9b00f3c04f
--- /dev/null
+++ b/app/models/safe_diffs/base.rb
@@ -0,0 +1,28 @@
+module SafeDiffs
+  class Base
+    attr_reader :diff_refs
+    attr_reader :project
+
+    def initialize(diffs, project: project, diff_refs: nil)
+      @diffs = diffs
+      @diff_refs = diff_refs
+      @project = project
+    end
+
+    def diff_files
+      @diff_files ||= @diffs.decorate! do |diff|
+        Gitlab::Diff::File.new(diff, diff_refs: @diff_refs, repository: @project.repository)
+      end
+    end
+
+    def highlighted_diff_files
+      diff_files
+    end
+
+    def count
+      # Used in commit#builds probably we need to go to real_size for consistency
+      # Yes I'm almost sure has to be real_size
+      @diffs.real_size
+    end
+  end
+end
diff --git a/app/models/safe_diffs/commit.rb b/app/models/safe_diffs/commit.rb
new file mode 100644
index 00000000000..c6f454f497f
--- /dev/null
+++ b/app/models/safe_diffs/commit.rb
@@ -0,0 +1,7 @@
+module SafeDiffs
+  class Commit < Base
+    def initialize(commit, diff_options: diff_options)
+      super(commit.diffs(diff_options), project: commit.project, diff_refs: commit.diff_refs)
+    end
+  end
+end
diff --git a/app/models/safe_diffs/compare.rb b/app/models/safe_diffs/compare.rb
new file mode 100644
index 00000000000..2b9b4da3958
--- /dev/null
+++ b/app/models/safe_diffs/compare.rb
@@ -0,0 +1,7 @@
+module SafeDiffs
+  class Compare < Base
+    def initialize(compare, project: project, diff_options: diff_options, diff_refs: nil)
+      super(compare.diffs(diff_options), project: project, diff_refs: diff_refs)
+    end
+  end
+end
diff --git a/app/models/safe_diffs/merge_request.rb b/app/models/safe_diffs/merge_request.rb
new file mode 100644
index 00000000000..4de9bbae94d
--- /dev/null
+++ b/app/models/safe_diffs/merge_request.rb
@@ -0,0 +1,43 @@
+module SafeDiffs
+  class MergeRequest < Base
+    def initialize(merge_request, diff_options: diff_options)
+      @merge_request = merge_request
+      @diff_options = diff_options
+
+      super(merge_request.diffs(diff_options), diff_refs: merge_request.diff_refs, project: merge_request.project)
+    end
+
+    def highlighted_diff_files
+      diffs = diff_files
+      # Based on diff_options too
+      cache_highlighted_diff_lines = Rails.cache.read(cache_key) || {}
+      store_highlighted_diff_lines = cache_highlighted_diff_lines.empty?
+
+      diffs.each do |diff_file|
+        file_path = diff_file.file_path
+
+        if cache_highlighted_diff_lines[file_path]
+          diff_file.diff_lines = cache_highlighted_diff_lines[file_path].map do |highlighted_line|
+            Gitlab::Diff::Line.init_from_hash(highlighted_line)
+          end
+          diff_file.highlighted_diff_lines = diff_file.diff_lines
+        else
+          store_highlighted_diff_lines = true
+          cache_highlighted_diff_lines[file_path] ||= diff_file.highlighted_diff_lines.map(&:to_hash)
+        end
+      end
+
+      if store_highlighted_diff_lines
+        Rails.cache.write(cache_key, cache_highlighted_diff_lines)
+      end
+
+      diffs
+    end
+
+    private
+
+    def cache_key
+      [@merge_request, 'highlighted-safe-diff-files', @diff_options]
+    end
+  end
+end
diff --git a/app/views/projects/commit/show.html.haml b/app/views/projects/commit/show.html.haml
index d0da2606587..11b2020f99b 100644
--- a/app/views/projects/commit/show.html.haml
+++ b/app/views/projects/commit/show.html.haml
@@ -7,7 +7,7 @@
   = render "ci_menu"
 - else
   %div.block-connector
-= render "projects/diffs/diffs", diffs: @diffs, project: @project, diff_refs: @commit.diff_refs
+= render "projects/diffs/diffs", diff_files: @diffs.diff_files, project: @diffs.project, diff_refs: @diffs.diff_refs
 = render "projects/notes/notes_with_form"
 - if can_collaborate_with_project?
   - %w(revert cherry-pick).each do |type|
diff --git a/app/views/projects/compare/show.html.haml b/app/views/projects/compare/show.html.haml
index 28a50e7031a..eb8a1bd5289 100644
--- a/app/views/projects/compare/show.html.haml
+++ b/app/views/projects/compare/show.html.haml
@@ -8,7 +8,7 @@
 
   - if @commits.present?
     = render "projects/commits/commit_list"
-    = render "projects/diffs/diffs", diffs: @diffs, project: @project, diff_refs: @diff_refs
+    = render "projects/diffs/diffs", diff_files: @diffs.diff_files, project: @diffs.project, diff_refs: @diffs.diff_refs
   - else
     .light-well
       .center
diff --git a/app/views/projects/diffs/_diffs.html.haml b/app/views/projects/diffs/_diffs.html.haml
index 8f40e8fce02..8bfb6ed0689 100644
--- a/app/views/projects/diffs/_diffs.html.haml
+++ b/app/views/projects/diffs/_diffs.html.haml
@@ -1,5 +1,4 @@
 - show_whitespace_toggle = local_assigns.fetch(:show_whitespace_toggle, true)
-- diff_files = local_assigns.fetch(:diff_files) { safe_diff_files(diffs, diff_refs: diff_refs, repository: project.repository) }
 - if diff_view == 'parallel'
   - fluid_layout true
 
diff --git a/app/views/projects/diffs/_file.html.haml b/app/views/projects/diffs/_file.html.haml
index c306909fb1a..a4b4391127e 100644
--- a/app/views/projects/diffs/_file.html.haml
+++ b/app/views/projects/diffs/_file.html.haml
@@ -16,4 +16,4 @@
 
         = view_file_btn(diff_commit.id, diff_file, project)
 
-  = render 'projects/diffs/content', diff_file: diff_file, diff_commit: diff_commit, diff_refs: diff_refs, blob: blob, project: project
+  = render 'projects/diffs/content', diff_file: diff_file, diff_commit: diff_commit, blob: blob, project: project
diff --git a/app/views/projects/merge_requests/_new_submit.html.haml b/app/views/projects/merge_requests/_new_submit.html.haml
index a5e67b95727..cfc647b6fae 100644
--- a/app/views/projects/merge_requests/_new_submit.html.haml
+++ b/app/views/projects/merge_requests/_new_submit.html.haml
@@ -31,7 +31,7 @@
     %li.diffs-tab.active
       = link_to url_for(params), data: {target: 'div#diffs', action: 'diffs', toggle: 'tab'} do
         Changes
-        %span.badge= @diffs.real_size
+        %span.badge= @diffs.count
 
   .tab-content
     #commits.commits.tab-pane
@@ -42,7 +42,7 @@
           %h4 This comparison includes more than #{MergeRequestDiff::COMMITS_SAFE_SIZE} commits.
           %p To preserve performance the line changes are not shown.
       - else
-        = render "projects/diffs/diffs", diffs: @diffs, project: @project, diff_refs: @merge_request.diff_refs, show_whitespace_toggle: false
+        = render "projects/diffs/diffs", diff_files: @diffs.diff_files, project: @diffs.project, diff_refs: @merge_request.diff_refs, show_whitespace_toggle: false
     - if @pipeline
       #builds.builds.tab-pane
         = render "projects/merge_requests/show/builds"
diff --git a/app/views/projects/merge_requests/show/_diffs.html.haml b/app/views/projects/merge_requests/show/_diffs.html.haml
index 94bf2e9707b..d8c28e875c0 100644
--- a/app/views/projects/merge_requests/show/_diffs.html.haml
+++ b/app/views/projects/merge_requests/show/_diffs.html.haml
@@ -1,7 +1,7 @@
 - if @merge_request_diff.collected?
-  - # REVIEW the problem of this is that it needs the variables use for the safe_diff_files helper
-  = render "projects/diffs/diffs", diff_files: @merge_request.highlighted_safe_diff_files(diff_options),
-    diff_refs: @merge_request.diff_refs, project: @merge_request.project
+  - diffs = SafeDiffs::MergeRequest.new(@merge_request, diff_options: diff_options)
+  = render "projects/diffs/diffs", diff_files: diffs.highlighted_diff_files,
+    diff_refs: diffs.diff_refs, project: diffs.project
 - 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/lib/gitlab/email/message/repository_push.rb b/lib/gitlab/email/message/repository_push.rb
index 97701b0cd42..48946ba355b 100644
--- a/lib/gitlab/email/message/repository_push.rb
+++ b/lib/gitlab/email/message/repository_push.rb
@@ -41,7 +41,7 @@ module Gitlab
         def diffs
           return unless compare
           
-          @diffs ||= safe_diff_files(compare.diffs(max_files: 30), diff_refs: diff_refs, repository: project.repository)
+          @diffs ||= SafeDiffs::Compare.new(compare, diff_options: { max_files: 30 }, project: project, diff_refs: diff_refs).diff_files
         end
 
         def diffs_count
diff --git a/spec/controllers/projects/commit_controller_spec.rb b/spec/controllers/projects/commit_controller_spec.rb
index 3001d32e719..952dab8db60 100644
--- a/spec/controllers/projects/commit_controller_spec.rb
+++ b/spec/controllers/projects/commit_controller_spec.rb
@@ -92,7 +92,7 @@ describe Projects::CommitController do
       let(:format) { :diff }
 
       it "should really only be a git diff" do
-        go(id: commit.id, format: format)
+        go(id: '66eceea0db202bb39c4e445e8ca28689645366c5', format: format)
 
         expect(response.body).to start_with("diff --git")
       end
@@ -101,8 +101,9 @@ describe Projects::CommitController do
         go(id: '66eceea0db202bb39c4e445e8ca28689645366c5', format: format, w: 1)
 
         expect(response.body).to start_with("diff --git")
-        # without whitespace option, there are more than 2 diff_splits
-        diff_splits = assigns(:diffs).first.diff.split("\n")
+
+        # without whitespace option, there are more than 2 diff_splits for other formats
+        diff_splits = assigns(:diffs).diff_files.first.diff.diff.split("\n")
         expect(diff_splits.length).to be <= 2
       end
     end
@@ -275,9 +276,9 @@ describe Projects::CommitController do
           end
 
           it 'only renders the diffs for the path given' do
-            expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs, diff_refs, project|
-              expect(diffs.map(&:new_path)).to contain_exactly(existing_path)
-              meth.call(diffs, diff_refs, project)
+            expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, safe_diffs|
+              expect(safe_diffs.diff_files.map(&:new_path)).to contain_exactly(existing_path)
+              meth.call(safe_diffs)
             end
 
             diff_for_path(id: commit.id, old_path: existing_path, new_path: existing_path)
diff --git a/spec/controllers/projects/compare_controller_spec.rb b/spec/controllers/projects/compare_controller_spec.rb
index 4058d5e2453..6272a5f111d 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).first).not_to be_nil
+    expect(assigns(:diffs).diff_files.first).not_to 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).first).not_to be_nil
+    expect(assigns(:diffs).diff_files.first).not_to be_nil
     expect(assigns(:commits).length).to be >= 1
     # without whitespace option, there are more than 2 diff_splits
-    diff_splits = assigns(:diffs).first.diff.split("\n")
+    diff_splits = assigns(:diffs).diff_files.first.diff.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_a).to eq([])
+      expect(assigns(:diffs).diff_files.to_a).to eq([])
       expect(assigns(:commits)).to eq([])
     end
 
@@ -87,9 +87,9 @@ describe Projects::CompareController do
           end
 
           it 'only renders the diffs for the path given' do
-            expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs, diff_refs, project|
-              expect(diffs.map(&:new_path)).to contain_exactly(existing_path)
-              meth.call(diffs, diff_refs, project)
+            expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, safe_diffs|
+              expect(safe_diffs.diff_files.map(&:new_path)).to contain_exactly(existing_path)
+              meth.call(safe_diffs)
             end
 
             diff_for_path(from: ref_from, to: ref_to, old_path: existing_path, new_path: existing_path)
diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index 210085e3b1a..9da43c9cee7 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -392,9 +392,9 @@ describe Projects::MergeRequestsController do
             end
 
             it 'only renders the diffs for the path given' do
-              expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs, diff_refs, project|
-                expect(diffs.map(&:new_path)).to contain_exactly(existing_path)
-                meth.call(diffs, diff_refs, project)
+              expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, safe_diffs|
+                expect(safe_diffs.diff_files.map(&:new_path)).to contain_exactly(existing_path)
+                meth.call(safe_diffs)
               end
 
               diff_for_path(id: merge_request.iid, old_path: existing_path, new_path: existing_path)
@@ -455,9 +455,9 @@ describe Projects::MergeRequestsController do
         end
 
         it 'only renders the diffs for the path given' do
-          expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs, diff_refs, project|
-            expect(diffs.map(&:new_path)).to contain_exactly(existing_path)
-            meth.call(diffs, diff_refs, project)
+          expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, safe_diffs|
+            expect(safe_diffs.diff_files.map(&:new_path)).to contain_exactly(existing_path)
+            meth.call(safe_diffs)
           end
 
           diff_for_path(old_path: existing_path, new_path: existing_path, merge_request: { source_branch: 'feature', target_branch: 'master' })
@@ -477,9 +477,9 @@ describe Projects::MergeRequestsController do
           end
 
           it 'only renders the diffs for the path given' do
-            expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs, diff_refs, project|
-              expect(diffs.map(&:new_path)).to contain_exactly(existing_path)
-              meth.call(diffs, diff_refs, project)
+            expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, safe_diffs|
+              expect(safe_diffs.diff_files.map(&:new_path)).to contain_exactly(existing_path)
+              meth.call(safe_diffs)
             end
 
             diff_for_path(old_path: existing_path, new_path: existing_path, merge_request: { source_project: other_project, source_branch: 'feature', target_branch: 'master' })