Clean up diff rendering

13 files changed, 121 insertions, 134 deletions

diff --git a/app/assets/javascripts/single_file_diff.js b/app/assets/javascripts/single_file_diff.js
index bacb26734c9..c44892dae3d 100644
--- a/app/assets/javascripts/single_file_diff.js
+++ b/app/assets/javascripts/single_file_diff.js
@@ -4,7 +4,7 @@
   window.SingleFileDiff = (function() {
     var COLLAPSED_HTML, ERROR_HTML, LOADING_HTML, WRAPPER;
 
-    WRAPPER = '<div class="diff-content diff-wrap-lines"></div>';
+    WRAPPER = '<div class="diff-content"></div>';
 
     LOADING_HTML = '<i class="fa fa-spinner fa-spin"></i>';
 
diff --git a/app/assets/stylesheets/pages/diff.scss b/app/assets/stylesheets/pages/diff.scss
index 6c935d58b73..58715c4c083 100644
--- a/app/assets/stylesheets/pages/diff.scss
+++ b/app/assets/stylesheets/pages/diff.scss
@@ -151,10 +151,6 @@
         }
       }
     }
-
-    .text-file.diff-wrap-lines table .line_holder td span {
-      white-space: pre-wrap;
-    }
   }
 
   .image {
diff --git a/app/controllers/concerns/diff_for_path.rb b/app/controllers/concerns/diff_for_path.rb
index 1efa9fe060f..d5388c4cd20 100644
--- a/app/controllers/concerns/diff_for_path.rb
+++ b/app/controllers/concerns/diff_for_path.rb
@@ -8,17 +8,6 @@ module DiffForPath
 
     return render_404 unless diff_file
 
-    diff_commit = commit_for_diff(diff_file)
-    blob = diff_file.blob(diff_commit)
-
-    locals = {
-      diff_file: diff_file,
-      diff_commit: diff_commit,
-      diff_refs: diffs.diff_refs,
-      blob: blob,
-      project: project
-    }
-
-    render json: { html: view_to_html_string('projects/diffs/_content', locals) }
+    render json: { html: view_to_html_string('projects/diffs/_content', diff_file: diff_file) }
   end
 end
diff --git a/app/helpers/commits_helper.rb b/app/helpers/commits_helper.rb
index d59d51905a6..5b5cdebe919 100644
--- a/app/helpers/commits_helper.rb
+++ b/app/helpers/commits_helper.rb
@@ -15,16 +15,6 @@ module CommitsHelper
     commit_person_link(commit, options.merge(source: :committer))
   end
 
-  def image_diff_class(diff)
-    if diff.deleted_file
-      "deleted"
-    elsif diff.new_file
-      "added"
-    else
-      nil
-    end
-  end
-
   def commit_to_html(commit, ref, project)
     render 'projects/commits/commit',
       commit: commit,
diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb
index da5838247a6..4cfaa103741 100644
--- a/app/helpers/diff_helper.rb
+++ b/app/helpers/diff_helper.rb
@@ -102,16 +102,32 @@ module DiffHelper
     ].join(' ').html_safe
   end
 
-  def commit_for_diff(diff_file)
-    return diff_file.content_commit if diff_file.content_commit
+  def diff_content_commit(diff_file)
+    content_commit = diff_file.content_commit
+    return content_commit if content_commit
 
-    if diff_file.deleted_file
-      @base_commit || @commit.parent || @commit
+    if diff_file.deleted_file?
+      diff_old_content_commit(diff_file)
     else
       @commit
     end
   end
 
+  def diff_old_content_commit(diff_file)
+    return if diff_file.new_file?
+
+    diff_file.old_content_commit || @base_commit || @commit.parent || @commit
+  end
+
+  def diff_file_blob_raw_path(diff_file)
+    namespace_project_raw_path(@project.namespace, @project, tree_join(diff_content_commit(diff_file).sha, diff_file.file_path))
+  end
+
+  def diff_file_old_blob_raw_path(diff_file)
+    return if diff_file.new_file?
+    namespace_project_raw_path(@project.namespace, @project, tree_join(diff_old_content_commit(diff_file).sha, diff_file.old_path))
+  end
+
   def diff_file_html_data(project, diff_file_path, diff_commit_id)
     {
       blob_diff_path: namespace_project_blob_diff_path(project.namespace, project,
diff --git a/app/views/projects/diffs/_content.html.haml b/app/views/projects/diffs/_content.html.haml
index a18451d8986..b361619fc79 100644
--- a/app/views/projects/diffs/_content.html.haml
+++ b/app/views/projects/diffs/_content.html.haml
@@ -1,12 +1,15 @@
-.diff-content.diff-wrap-lines
-  -# Skip all non non-supported blobs
-  - return unless blob.respond_to?(:text?)
+- diff_commit     = local_assigns.fetch(:diff_commit) { diff_content_commit(diff_file) }
+- diff_old_commit = local_assigns.fetch(:diff_old_commit) { diff_old_content_commit(diff_file) }
+- blob            = local_assigns.fetch(:blob) { diff_file.blob(diff_commit) }
+- old_blob        = local_assigns.fetch(:old_blob) { diff_file.old_blob(diff_old_commit) }
+
+.diff-content
   - if diff_file.too_large?
     .nothing-here-block This diff could not be displayed because it is too large.
   - elsif blob.too_large?
     .nothing-here-block The file could not be displayed because it is too large.
   - elsif blob.readable_text?
-    - if !project.repository.diffable?(blob)
+    - if !diff_file.repository.diffable?(blob)
       .nothing-here-block This diff was suppressed by a .gitattributes entry.
     - elsif diff_file.collapsed?
       - url = url_for(params.merge(action: :diff_for_path, old_path: diff_file.old_path, new_path: diff_file.new_path, file_identifier: diff_file.file_identifier))
@@ -15,20 +18,13 @@
         %a.click-to-expand
           Click to expand it.
     - elsif diff_file.diff_lines.length > 0
-      - total_lines = 0
-      - if blob.lines.any?
-        - total_lines = blob.lines.last.chomp == '' ? blob.lines.size - 1 : blob.lines.size
-      - if diff_view == :parallel
-        = render "projects/diffs/parallel_view", diff_file: diff_file, total_lines: total_lines
-      - else
-        = render "projects/diffs/text_file", diff_file: diff_file, total_lines: total_lines
+      = render "projects/diffs/viewers/text", diff_file: diff_file, blob: blob
     - else
       - if diff_file.mode_changed?
         .nothing-here-block File mode changed
       - elsif diff_file.renamed_file?
         .nothing-here-block File moved
   - elsif blob.image?
-    - old_blob = diff_file.old_blob(diff_file.old_content_commit || @base_commit)
-    = render "projects/diffs/image", diff_file: diff_file, old_file: old_blob, file: blob
+    = render "projects/diffs/viewers/image", diff_file: diff_file, blob: blob, old_blob: old_blob
   - else
     .nothing-here-block No preview for this file type
diff --git a/app/views/projects/diffs/_diffs.html.haml b/app/views/projects/diffs/_diffs.html.haml
index 71a1b9e6c05..4768438c29e 100644
--- a/app/views/projects/diffs/_diffs.html.haml
+++ b/app/views/projects/diffs/_diffs.html.haml
@@ -23,12 +23,4 @@
   = render 'projects/diffs/warning', diff_files: diffs
 
 .files{ data: { can_create_note: can_create_note } }
-  - diff_files.each_with_index do |diff_file|
-    - diff_commit = commit_for_diff(diff_file)
-    - blob = diff_file.blob(diff_commit)
-    - next unless blob
-    - blob.load_all_data!(diffs.project.repository) unless blob.too_large?
-    - file_hash = hexdigest(diff_file.file_path)
-
-    = render 'projects/diffs/file', file_hash: file_hash, project: diffs.project,
-      diff_file: diff_file, diff_commit: diff_commit, blob: blob, environment: environment
+  = render partial: 'projects/diffs/file', collection: diff_files, as: :diff_file, locals: { project: diffs.project, environment: environment }
diff --git a/app/views/projects/diffs/_file.html.haml b/app/views/projects/diffs/_file.html.haml
index f22b385fc0f..35650dee68e 100644
--- a/app/views/projects/diffs/_file.html.haml
+++ b/app/views/projects/diffs/_file.html.haml
@@ -1,8 +1,11 @@
 - environment = local_assigns.fetch(:environment, nil)
+- diff_commit = diff_content_commit(diff_file)
+- blob = diff_file.blob(diff_commit)
+- file_hash = hexdigest(diff_file.file_path)
 .diff-file.file-holder{ id: file_hash, data: diff_file_html_data(project, diff_file.file_path, diff_commit.id) }
   .js-file-title.file-title-flex-parent
     .file-header-content
-      = render "projects/diffs/file_header", diff_file: diff_file, blob: blob, diff_commit: diff_commit, project: project, url: "##{file_hash}"
+      = render "projects/diffs/file_header", diff_file: diff_file, blob: blob, diff_commit: diff_commit, url: "##{file_hash}"
 
     - unless diff_file.submodule?
       .file-actions.hidden-xs
@@ -15,9 +18,9 @@
             = edit_blob_link(@merge_request.source_project, @merge_request.source_branch, diff_file.new_path,
                              blob: blob, link_opts: link_opts)
 
-        = view_file_button(diff_commit.id, diff_file.new_path, project)
-        = view_on_environment_button(diff_commit.id, diff_file.new_path, environment) if environment
+        = view_file_button(diff_commit.id, diff_file.file_path, project)
+        = view_on_environment_button(diff_commit.id, diff_file.file_path, environment) if environment
 
   = render 'projects/fork_suggestion'
 
-  = render 'projects/diffs/content', diff_file: diff_file, diff_commit: diff_commit, blob: blob, project: project
+  = render 'projects/diffs/content', diff_file: diff_file, diff_commit: diff_commit, blob: blob
diff --git a/app/views/projects/diffs/_file_header.html.haml b/app/views/projects/diffs/_file_header.html.haml
index 4e4fdb73ae3..3ce963e06ba 100644
--- a/app/views/projects/diffs/_file_header.html.haml
+++ b/app/views/projects/diffs/_file_header.html.haml
@@ -3,19 +3,19 @@
 - if show_toggle
   %i.fa.diff-toggle-caret.fa-fw
 
-- if defined?(blob) && blob && diff_file.submodule?
+- if diff_file.submodule?
   %span
     = icon('archive fw')
 
     %strong.file-title-name
-      = submodule_link(blob, diff_commit.id, project.repository)
+      = submodule_link(blob, diff_commit.id, diff_file.repository)
 
     = copy_file_path_button(blob.path)
 - else
   = conditional_link_to url.present?, url do
     = blob_icon diff_file.b_mode, diff_file.file_path
 
-    - if diff_file.renamed_file
+    - if diff_file.renamed_file?
       - old_path, new_path = mark_inline_diffs(diff_file.old_path, diff_file.new_path)
       %strong.file-title-name.has-tooltip{ data: { title: diff_file.old_path, container: 'body' } }
         = old_path
@@ -23,12 +23,13 @@
       %strong.file-title-name.has-tooltip{ data: { title: diff_file.new_path, container: 'body' } }
         = new_path
     - else
-      %strong.file-title-name.has-tooltip{ data: { title: diff_file.new_path, container: 'body' } }
-        = diff_file.new_path
-      - if diff_file.deleted_file
+      %strong.file-title-name.has-tooltip{ data: { title: diff_file.file_path, container: 'body' } }
+        = diff_file.file_path
+
+      - if diff_file.deleted_file?
         deleted
 
-  = copy_file_path_button(diff_file.new_path)
+  = copy_file_path_button(diff_file.file_path)
 
   - if diff_file.mode_changed?
     %small
diff --git a/app/views/projects/diffs/_image.html.haml b/app/views/projects/diffs/_image.html.haml
deleted file mode 100644
index ca10921c5e2..00000000000
--- a/app/views/projects/diffs/_image.html.haml
+++ /dev/null
@@ -1,69 +0,0 @@
-- diff = diff_file.diff
-- file_raw_path = namespace_project_raw_path(@project.namespace, @project, tree_join(diff_file.new_ref, diff.new_path))
-// diff_refs will be nil for orphaned commits (e.g. first commit in repo)
-- if diff_file.old_ref
-  - old_file_raw_path = namespace_project_raw_path(@project.namespace, @project, tree_join(diff_file.old_ref, diff.old_path))
-
-- if diff.renamed_file || diff.new_file || diff.deleted_file
-  .image
-    %span.wrap
-      .frame{ class: image_diff_class(diff) }
-        %img{ src: diff.deleted_file ? old_file_raw_path : file_raw_path, alt: diff.new_path }
-      %p.image-info= number_to_human_size(file.size)
-- else
-  .image
-    .two-up.view
-      %span.wrap
-        .frame.deleted
-          %a{ href: namespace_project_blob_path(@project.namespace, @project, tree_join(diff_file.old_ref, diff.old_path)) }
-            %img{ src: old_file_raw_path, alt: diff.old_path }
-        %p.image-info.hide
-          %span.meta-filesize= number_to_human_size(old_file.size)
-          |
-          %b W:
-          %span.meta-width
-          |
-          %b H:
-          %span.meta-height
-      %span.wrap
-        .frame.added
-          %a{ href: namespace_project_blob_path(@project.namespace, @project, tree_join(diff_file.new_ref, diff.new_path)) }
-            %img{ src: file_raw_path, alt: diff.new_path }
-        %p.image-info.hide
-          %span.meta-filesize= number_to_human_size(file.size)
-          |
-          %b W:
-          %span.meta-width
-          |
-          %b H:
-          %span.meta-height
-
-    .swipe.view.hide
-      .swipe-frame
-        .frame.deleted
-          %img{ src: old_file_raw_path, alt: diff.old_path }
-        .swipe-wrap
-          .frame.added
-            %img{ src: file_raw_path, alt: diff.new_path }
-        %span.swipe-bar
-          %span.top-handle
-          %span.bottom-handle
-
-    .onion-skin.view.hide
-      .onion-skin-frame
-        .frame.deleted
-          %img{ src: old_file_raw_path, alt: diff.old_path }
-        .frame.added
-          %img{ src: file_raw_path, alt: diff.new_path }
-        .controls
-          .transparent
-          .drag-track
-            .dragger{ :style => "left: 0px;" }
-          .opaque
-
-
-  .view-modes.hide
-    %ul.view-modes-menu
-      %li.two-up{ data: { mode: 'two-up' } } 2-up
-      %li.swipe{ data: { mode: 'swipe' } } Swipe
-      %li.onion-skin{ data: { mode: 'onion-skin' } } Onion skin
diff --git a/app/views/projects/diffs/_text_file.html.haml b/app/views/projects/diffs/_text_file.html.haml
index 5528014095c..e8a5e63e59e 100644
--- a/app/views/projects/diffs/_text_file.html.haml
+++ b/app/views/projects/diffs/_text_file.html.haml
@@ -3,7 +3,7 @@
   .suppressed-container
     %a.show-suppressed-diff.js-show-suppressed-diff Changes suppressed. Click to show.
 
-%table.text-file.code.js-syntax-highlight{ data: diff_view_data, class: too_big ? 'hide' : '' }
+%table.text-file.diff-wrap-lines.code.js-syntax-highlight{ data: diff_view_data, class: too_big ? 'hide' : '' }
   = render partial: "projects/diffs/line",
     collection: diff_file.highlighted_diff_lines,
     as: :line,
diff --git a/app/views/projects/diffs/viewers/_image.html.haml b/app/views/projects/diffs/viewers/_image.html.haml
new file mode 100644
index 00000000000..b42904a6139
--- /dev/null
+++ b/app/views/projects/diffs/viewers/_image.html.haml
@@ -0,0 +1,66 @@
+- blob_raw_path = diff_file_blob_raw_path(diff_file)
+- old_blob_raw_path = diff_file_old_blob_raw_path(diff_file)
+
+- if diff_file.new_file? || diff_file.deleted_file?
+  .image
+    %span.wrap
+      .frame{ class: (diff_file.deleted_file? ? 'deleted' : 'added') }
+        %img{ src: blob_raw_path, alt: diff_file.file_path }
+      %p.image-info= number_to_human_size(blob.size)
+- else
+  .image
+    .two-up.view
+      %span.wrap
+        .frame.deleted
+          %a{ href: namespace_project_blob_path(@project.namespace, @project, tree_join(diff_old_content_commit(diff_file).sha, diff_file.old_path)) }
+            %img{ src: old_blob_raw_path, alt: diff_file.old_path }
+        %p.image-info.hide
+          %span.meta-filesize= number_to_human_size(old_blob.size)
+          |
+          %b W:
+          %span.meta-width
+          |
+          %b H:
+          %span.meta-height
+      %span.wrap
+        .frame.added
+          %a{ href: namespace_project_blob_path(@project.namespace, @project, tree_join(diff_content_commit(diff_file).sha, diff_file.new_path)) }
+            %img{ src: blob_raw_path, alt: diff_file.new_path }
+        %p.image-info.hide
+          %span.meta-filesize= number_to_human_size(blob.size)
+          |
+          %b W:
+          %span.meta-width
+          |
+          %b H:
+          %span.meta-height
+
+    .swipe.view.hide
+      .swipe-frame
+        .frame.deleted
+          %img{ src: old_blob_raw_path, alt: diff_file.old_path }
+        .swipe-wrap
+          .frame.added
+            %img{ src: blob_raw_path, alt: diff_file.new_path }
+        %span.swipe-bar
+          %span.top-handle
+          %span.bottom-handle
+
+    .onion-skin.view.hide
+      .onion-skin-frame
+        .frame.deleted
+          %img{ src: old_blob_raw_path, alt: diff_file.old_path }
+        .frame.added
+          %img{ src: blob_raw_path, alt: diff_file.new_path }
+        .controls
+          .transparent
+          .drag-track
+            .dragger{ :style => "left: 0px;" }
+          .opaque
+
+
+  .view-modes.hide
+    %ul.view-modes-menu
+      %li.two-up{ data: { mode: 'two-up' } } 2-up
+      %li.swipe{ data: { mode: 'swipe' } } Swipe
+      %li.onion-skin{ data: { mode: 'onion-skin' } } Onion skin
diff --git a/app/views/projects/diffs/viewers/_text.html.haml b/app/views/projects/diffs/viewers/_text.html.haml
new file mode 100644
index 00000000000..248fa52269d
--- /dev/null
+++ b/app/views/projects/diffs/viewers/_text.html.haml
@@ -0,0 +1,7 @@
+- blob.load_all_data!(diff_file.repository)
+- total_lines = blob.lines.size
+- total_lines -= 1 if total_lines > 0 && blob.lines.last.blank?
+- if diff_view == :parallel
+  = render "projects/diffs/parallel_view", diff_file: diff_file, total_lines: total_lines
+- else
+  = render "projects/diffs/text_file", diff_file: diff_file, total_lines: total_lines