Clean up diff rendering

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>';

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 {

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

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,

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
-    if diff_file.deleted_file
-      @base_commit || @commit.parent || @commit
+  def diff_content_commit(diff_file)
+    content_commit = diff_file.content_commit
+    return content_commit if content_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,

app/views/projects/diffs/_content.html.haml

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

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 }

app/views/projects/diffs/_file.html.haml

 - 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

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

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,

app/views/projects/diffs/_image.html.haml → app/views/projects/diffs/viewers/_image.html.haml

-- 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
+- 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: 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)
+      .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_file.old_ref, diff.old_path)) }
-            %img{ src: old_file_raw_path, alt: diff.old_path }
+          %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_file.size)
+          %span.meta-filesize= number_to_human_size(old_blob.size)
           |
           %b W:
           %span.meta-width
@@ -27,10 +24,10 @@
           %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 }
+          %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(file.size)
+          %span.meta-filesize= number_to_human_size(blob.size)
           |
           %b W:
           %span.meta-width
@@ -41,10 +38,10 @@
     .swipe.view.hide
       .swipe-frame
         .frame.deleted
-          %img{ src: old_file_raw_path, alt: diff.old_path }
+          %img{ src: old_blob_raw_path, alt: diff_file.old_path }
         .swipe-wrap
           .frame.added
-            %img{ src: file_raw_path, alt: diff.new_path }
+            %img{ src: blob_raw_path, alt: diff_file.new_path }
         %span.swipe-bar
           %span.top-handle
           %span.bottom-handle
@@ -52,9 +49,9 @@
     .onion-skin.view.hide
       .onion-skin-frame
         .frame.deleted
-          %img{ src: old_file_raw_path, alt: diff.old_path }
+          %img{ src: old_blob_raw_path, alt: diff_file.old_path }
         .frame.added
-          %img{ src: file_raw_path, alt: diff.new_path }
+          %img{ src: blob_raw_path, alt: diff_file.new_path }
         .controls
           .transparent
           .drag-track

app/views/projects/diffs/viewers/_text.html.haml

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

lib/gitlab/git/diff_collection.rb

@@ -64,6 +64,8 @@ module Gitlab
         collection
       end
+      alias_method :to_ary, :to_a
+
       private
       def populate!