Merge branch 'dm-diff-cleanup' into 'master'

Clean up diff rendering

See merge request !11390

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/framework/files.scss

@@ -66,10 +66,10 @@
     &.video {
       background: $file-image-bg;
       text-align: center;
+      padding: 30px;
 
       img,
       video {
-        padding: 20px;
         max-width: 80%;
       }
     }

app/assets/stylesheets/framework/variables.scss

@@ -247,7 +247,6 @@ $dark-diff-match-bg: rgba(255, 255, 255, 0.3);
 $dark-diff-match-color: rgba(255, 255, 255, 0.1);
 $file-mode-changed: #777;
 $file-mode-changed: #777;
-$diff-image-bg: #ddd;
 $diff-image-info-color: grey;
 $diff-swipe-border: #999;
 $diff-view-modes-color: grey;

app/assets/stylesheets/pages/diff.scss

@@ -151,14 +151,10 @@
         }
       }
     }
-
-    .text-file.diff-wrap-lines table .line_holder td span {
-      white-space: pre-wrap;
-    }
   }
 
   .image {
-    background: $diff-image-bg;
+    background: $file-image-bg;
     text-align: center;
     padding: 30px;
 

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/controllers/projects/compare_controller.rb

@@ -51,13 +51,9 @@ class Projects::CompareController < Projects::ApplicationController
 
     if @compare
       @commits = @compare.commits
-      @start_commit = @compare.start_commit
-      @commit = @compare.commit
-      @base_commit = @compare.base_commit
-
       @diffs = @compare.diffs(diff_options)
 
-      environment_params = @repository.branch_exists?(@head_ref) ? { ref: @head_ref } : { commit: @commit }
+      environment_params = @repository.branch_exists?(@head_ref) ? { ref: @head_ref } : { commit: @compare.commit }
       @environment = EnvironmentsFinder.new(@project, current_user, environment_params).execute.last
 
       @diff_notes_disabled = true

app/controllers/projects/merge_requests_controller.rb

@@ -14,7 +14,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   ]
   before_action :validates_merge_request, only: [:show, :diffs, :commits, :pipelines]
   before_action :define_show_vars, only: [:diffs, :commits, :conflicts, :conflict_for_path, :builds, :pipelines]
-  before_action :define_commit_vars, only: [:diffs]
   before_action :ensure_ref_fetched, only: [:show, :diffs, :commits, :builds, :conflicts, :conflict_for_path, :pipelines]
   before_action :close_merge_request_without_source_project, only: [:show, :diffs, :commits, :builds, :pipelines]
   before_action :check_if_can_be_merged, only: :show
@@ -130,8 +129,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController
       @diff_notes_disabled = true
     end
 
-    define_commit_vars
-
     render_diff_for_path(@diffs)
   end
 
@@ -500,11 +497,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     @notes = prepare_notes_for_rendering(@discussions.flat_map(&:notes))
   end
 
-  def define_commit_vars
-    @commit = @merge_request.diff_head_commit
-    @base_commit = @merge_request.diff_base_commit || @merge_request.likely_diff_base_commit
-  end
-
   def define_diff_vars
     @merge_request_diff =
       if params[:diff_id]
@@ -569,7 +561,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     @source_project = merge_request.source_project
     @commits = @merge_request.compare_commits.reverse
     @commit = @merge_request.diff_head_commit
-    @base_commit = @merge_request.diff_base_commit
 
     @note_counts = Note.where(commit_id: @commits.map(&:id)).
       group(:commit_id).count

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,14 +102,14 @@ 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
-    else
-      @commit
+  def diff_file_blob_raw_path(diff_file)
+    namespace_project_raw_path(@project.namespace, @project, tree_join(diff_file.content_sha, diff_file.file_path))
   end
+
+  def diff_file_old_blob_raw_path(diff_file)
+    sha = diff_file.old_content_sha
+    return unless sha
+    namespace_project_raw_path(@project.namespace, @project, tree_join(diff_file.old_content_sha, diff_file.old_path))
   end
 
   def diff_file_html_data(project, diff_file_path, diff_commit_id)
@@ -120,8 +120,8 @@ module DiffHelper
     }
   end
 
-  def editable_diff?(diff)
-    !diff.deleted_file && @merge_request && @merge_request.source_project
+  def editable_diff?(diff_file)
+    !diff_file.deleted_file? && @merge_request && @merge_request.source_project
   end
 
   private

app/models/concerns/note_on_diff.rb

@@ -33,14 +33,4 @@ module NoteOnDiff
   def created_at_diff?(diff_refs)
     false
   end
-
-  private
-
-  def noteable_diff_refs
-    if noteable.respond_to?(:diff_sha_refs)
-      noteable.diff_sha_refs
-    else
-      noteable.diff_refs
-    end
-  end
 end

app/models/diff_note.rb

@@ -63,7 +63,7 @@ class DiffNote < Note
     return false unless supported?
     return true if for_commit?
 
-    diff_refs ||= noteable_diff_refs
+    diff_refs ||= noteable.diff_refs
 
     self.position.diff_refs == diff_refs
   end
@@ -99,7 +99,7 @@ class DiffNote < Note
       self.project,
       nil,
       old_diff_refs: self.position.diff_refs,
-      new_diff_refs: noteable_diff_refs,
+      new_diff_refs: noteable.diff_refs,
       paths: self.position.paths
     ).execute(self)
   end

app/models/legacy_diff_note.rb

@@ -61,7 +61,7 @@ class LegacyDiffNote < Note
     return true if for_commit?
     return true unless diff_line
     return false unless noteable
-    return false if diff_refs && diff_refs != noteable_diff_refs
+    return false if diff_refs && diff_refs != noteable.diff_refs
 
     noteable_diff = find_noteable_diff
 

app/models/merge_request.rb

@@ -245,19 +245,6 @@ class MergeRequest < ActiveRecord::Base
     end
   end
 
-  # MRs created before 8.4 don't store a MergeRequestDiff#base_commit_sha,
-  # but we need to get a commit for the "View file @ ..." link by deleted files,
-  # so we find the likely one if we can't get the actual one.
-  # This will not be the actual base commit if the target branch was merged into
-  # the source branch after the merge request was created, but it is good enough
-  # for the specific purpose of linking to a commit.
-  # It is not good enough for use in `Gitlab::Git::DiffRefs`, which needs the
-  # true base commit, so we can't simply have `#diff_base_commit` fall back on
-  # this method.
-  def likely_diff_base_commit
-    first_commit.try(:parent) || first_commit
-  end
-
   def diff_start_commit
     if persisted?
       merge_request_diff.start_commit
@@ -322,22 +309,15 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def diff_refs
-    return unless diff_start_commit || diff_base_commit
-
+    if persisted?
+      merge_request_diff.diff_refs
+    else
       Gitlab::Diff::DiffRefs.new(
         base_sha:  diff_base_sha,
         start_sha: diff_start_sha,
         head_sha:  diff_head_sha
       )
     end
-
-  # Return diff_refs instance trying to not touch the git repository
-  def diff_sha_refs
-    if merge_request_diff && merge_request_diff.diff_refs_by_sha?
-      merge_request_diff.diff_refs
-    else
-      diff_refs
-    end
   end
 
   def branch_merge_base_sha
@@ -870,7 +850,7 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def has_complete_diff_refs?
-    diff_sha_refs && diff_sha_refs.complete?
+    diff_refs && diff_refs.complete?
   end
 
   def update_diff_notes_positions(old_diff_refs:, new_diff_refs:, current_user: nil)

app/models/merge_request_diff.rb

@@ -150,6 +150,29 @@ class MergeRequestDiff < ActiveRecord::Base
     )
   end
 
+  # MRs created before 8.4 don't store their true diff refs (start and base),
+  # but we need to get a commit SHA for the "View file @ ..." link by a file,
+  # so we use an approximation of the diff refs if we can't get the actual one.
+  #
+  # These will not be the actual diff refs if the target branch was merged into
+  # the source branch after the merge request was created, but it is good enough
+  # for the specific purpose of linking to a commit.
+  #
+  # It is not good enough for highlighting diffs, so we can't simply pass
+  # these as `diff_refs.`
+  def fallback_diff_refs
+    real_refs = diff_refs
+    return real_refs if real_refs
+
+    likely_base_commit_sha = (first_commit&.parent || first_commit)&.sha
+
+    Gitlab::Diff::DiffRefs.new(
+      base_sha:  likely_base_commit_sha,
+      start_sha: safe_start_commit_sha,
+      head_sha:  head_commit_sha
+    )
+  end
+
   def diff_refs_by_sha?
     base_commit_sha? && head_commit_sha? && start_commit_sha?
   end

app/views/discussions/_diff_with_notes.html.haml

@@ -3,7 +3,7 @@
 
 .diff-file.file-holder
   .js-file-title.file-title
-    = render "projects/diffs/file_header", diff_file: diff_file, blob: blob, diff_commit: diff_file.content_commit, project: discussion.project, url: discussion_path(discussion), show_toggle: false
+    = render "projects/diffs/file_header", diff_file: diff_file, url: discussion_path(discussion), show_toggle: false
 
   .diff-content.code.js-syntax-highlight
     %table

app/views/notify/repository_push_email.html.haml

@@ -27,40 +27,38 @@
   %h4 #{pluralize @message.diffs_count, "changed file"}:
 
   %ul
-    - @message.diffs.each do |diff|
+    - @message.diffs.each do |diff_file|
       %li.file-stats
-        %a{ href: "#{@message.target_url if @message.disable_diffs?}##{hexdigest(diff.file_path)}" }
-          - if diff.deleted_file
+        %a{ href: "#{@message.target_url if @message.disable_diffs?}##{hexdigest(diff_file.file_path)}" }
+          - if diff_file.deleted_file?
             %span.deleted-file
               −
-              = diff.old_path
-          - elsif diff.renamed_file
-            = diff.old_path
+              = diff_file.old_path
+          - elsif diff_file.renamed_file?
+            = diff_file.old_path
             →
-            = diff.new_path
-          - elsif diff.new_file
+            = diff_file.new_path
+          - elsif diff_file.new_file?
             %span.new-file
               +
-              = diff.new_path
+              = diff_file.new_path
           - else
-            = diff.new_path
+            = diff_file.new_path
 
   - unless @message.disable_diffs?
-    - diff_files = @message.diffs
-
     - if @message.compare_timeout
       %h5 The diff was not included because it is too large.
     - else
       %h4 Changes:
-      - diff_files.each do |diff_file|
+      - @message.diffs.each do |diff_file|
         - file_hash = hexdigest(diff_file.file_path)
         %li{ id: file_hash }
           %a{ href: @message.target_url + "##{file_hash}" }<
-            - if diff_file.deleted_file
+            - if diff_file.deleted_file?
               %strong<
                 = diff_file.old_path
               deleted
-            - elsif diff_file.renamed_file
+            - elsif diff_file.renamed_file?
               %strong<
                 = diff_file.old_path
               &rarr;

app/views/notify/repository_push_email.text.haml

@@ -15,15 +15,15 @@
   \
   #{pluralize @message.diffs_count, "changed file"}:
   \
-  - @message.diffs.each do |diff|
-    - if diff.deleted_file
-      \- − #{diff.old_path}
-    - elsif diff.renamed_file
-      \- #{diff.old_path} → #{diff.new_path}
-    - elsif diff.new_file
-      \- + #{diff.new_path}
+  - @message.diffs.each do |diff_file|
+    - if diff_file.deleted_file?
+      \- − #{diff_file.old_path}
+    - elsif diff_file.renamed_file?
+      \- #{diff_file.old_path} → #{diff_file.new_path}
+    - elsif diff_file.new_file?
+      \- + #{diff_file.new_path}
     - else
-      \- #{diff.new_path}
+      \- #{diff_file.new_path}
   - unless @message.disable_diffs?
     - if @message.compare_timeout
       \
@@ -36,9 +36,9 @@
       - @message.diffs.each do |diff_file|
         \
         \=====================================
-        - if diff_file.deleted_file
+        - if diff_file.deleted_file?
           #{diff_file.old_path} deleted
-        - elsif diff_file.renamed_file
+        - elsif diff_file.renamed_file?
           #{diff_file.old_path} → #{diff_file.new_path}
         - else
           = diff_file.new_path

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

-.diff-content.diff-wrap-lines
-  -# Skip all non non-supported blobs
-  - return unless blob.respond_to?(:text?)
+- blob = diff_file.blob
+
+.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 +15,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
     - else
       - if diff_file.mode_changed?
         .nothing-here-block File mode changed
-      - elsif diff_file.renamed_file
+      - 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
   - 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-file.file-holder{ id: file_hash, data: diff_file_html_data(project, diff_file.file_path, diff_commit.id) }
+- 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_file.content_sha) }
   .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, url: "##{file_hash}"
 
     - unless diff_file.submodule?
+      - blob = diff_file.blob
       .file-actions.hidden-xs
         - if blob.readable_text?
           = link_to '#', class: 'js-toggle-diff-comments btn active has-tooltip', title: "Toggle comments for this file", disabled: @diff_notes_disabled do
@@ -15,9 +17,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_file.content_sha, diff_file.file_path, project)
+        = view_on_environment_button(diff_file.content_sha, 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

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

@@ -3,19 +3,20 @@
 - if show_toggle
   %i.fa.diff-toggle-caret.fa-fw
 
-- if defined?(blob) && blob && diff_file.submodule?
+- if diff_file.submodule?
+  - blob = diff_file.blob
   %span
     = icon('archive fw')
 
     %strong.file-title-name
-      = submodule_link(blob, diff_commit.id, project.repository)
+      = submodule_link(blob, diff_file.content_sha, 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 +24,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/_parallel_view.html.haml

@@ -49,7 +49,7 @@
 
       - if discussions_left || discussions_right
         = render "discussions/parallel_diff_discussion", discussions_left: discussions_left, discussions_right: discussions_right
-    - if !diff_file.new_file && !diff_file.deleted_file && diff_file.diff_lines.any?
+    - if !diff_file.new_file? && !diff_file.deleted_file? && diff_file.diff_lines.any?
       - last_line = diff_file.diff_lines.last
       - if last_line.new_pos < total_lines
         %tr.line_holder.parallel

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

@@ -12,19 +12,19 @@
       - diff_files.each do |diff_file|
         - file_hash = hexdigest(diff_file.file_path)
         %li
-          - if diff_file.deleted_file
+          - if diff_file.deleted_file?
             %span.deleted-file
               %a{ href: "##{file_hash}" }
                 %i.fa.fa-minus
                 = diff_file.old_path
-          - elsif diff_file.renamed_file
+          - elsif diff_file.renamed_file?
             %span.renamed-file
               %a{ href: "##{file_hash}" }
                 %i.fa.fa-minus
                 = diff_file.old_path
                 →
                 = diff_file.new_path
-          - elsif diff_file.new_file
+          - elsif diff_file.new_file?
             %span.new-file
               %a{ href: "##{file_hash}" }
                 %i.fa.fa-plus

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

@@ -3,13 +3,13 @@
   .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,
     locals: { diff_file: diff_file, discussions: @grouped_diff_discussions }
 
-  - if !diff_file.new_file && !diff_file.deleted_file && diff_file.highlighted_diff_lines.any?
+  - if !diff_file.new_file? && !diff_file.deleted_file? && diff_file.highlighted_diff_lines.any?
     - last_line = diff_file.highlighted_diff_lines.last
     - if last_line.new_pos < total_lines
       %tr.line_holder

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))
+- blob = diff_file.blob
+- old_blob = diff_file.old_blob
+- blob_raw_path = diff_file_blob_raw_path(diff_file)
+- old_blob_raw_path = diff_file_old_blob_raw_path(diff_file)
 
-- if diff.renamed_file || diff.new_file || diff.deleted_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_file.old_content_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 +26,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_file.content_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 +40,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 +51,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 = diff_file.blob
+- 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/api/entities.rb

@@ -252,7 +252,9 @@ module API
 
     class RepoDiff < Grape::Entity
       expose :old_path, :new_path, :a_mode, :b_mode, :diff
-      expose :new_file, :renamed_file, :deleted_file
+      expose :new_file?, as: :new_file
+      expose :renamed_file?, as: :renamed_file
+      expose :deleted_file?, as: :deleted_file
     end
 
     class Milestone < ProjectEntity

lib/gitlab/diff/file.rb

 module Gitlab
   module Diff
     class File
-      attr_reader :diff, :repository, :diff_refs
+      attr_reader :diff, :repository, :diff_refs, :fallback_diff_refs
 
-      delegate :new_file, :deleted_file, :renamed_file,
-        :old_path, :new_path, :a_mode, :b_mode,
+      delegate :new_file?, :deleted_file?, :renamed_file?,
+        :old_path, :new_path, :a_mode, :b_mode, :mode_changed?,
         :submodule?, :too_large?, :collapsed?, to: :diff, prefix: false
 
-      def initialize(diff, repository:, diff_refs: nil)
+      def initialize(diff, repository:, diff_refs: nil, fallback_diff_refs: nil)
         @diff = diff
         @repository = repository
         @diff_refs = diff_refs
+        @fallback_diff_refs = fallback_diff_refs
       end
 
       def position(line)
@@ -49,24 +50,60 @@ module Gitlab
         line_code(line) if line
       end
 
+      def old_sha
+        diff_refs&.base_sha
+      end
+
+      def new_sha
+        diff_refs&.head_sha
+      end
+
+      def content_sha
+        return old_content_sha if deleted_file?
+        return @content_sha if defined?(@content_sha)
+
+        refs = diff_refs || fallback_diff_refs
+        @content_sha = refs&.head_sha
+      end
+
       def content_commit
-        return unless diff_refs
+        return @content_commit if defined?(@content_commit)
+
+        sha = content_sha
+        @content_commit = repository.commit(sha) if sha
+      end
+
+      def old_content_sha
+        return if new_file?
+        return @old_content_sha if defined?(@old_content_sha)
 
-        repository.commit(deleted_file ? old_ref : new_ref)
+        refs = diff_refs || fallback_diff_refs
+        @old_content_sha = refs&.base_sha
       end
 
       def old_content_commit
-        return unless diff_refs
+        return @old_content_commit if defined?(@old_content_commit)
 
-        repository.commit(old_ref)
+        sha = old_content_sha
+        @old_content_commit = repository.commit(sha) if sha
       end
 
-      def old_ref
-        diff_refs.try(:base_sha)
+      def blob
+        return @blob if defined?(@blob)
+
+        sha = content_sha
+        return @blob = nil unless sha
+
+        repository.blob_at(sha, file_path)
       end
 
-      def new_ref
-        diff_refs.try(:head_sha)
+      def old_blob
+        return @old_blob if defined?(@old_blob)
+
+        sha = old_content_sha
+        return @old_blob = nil unless sha
+
+        @old_blob = repository.blob_at(sha, old_path)
       end
 
       attr_writer :highlighted_diff_lines
@@ -85,10 +122,6 @@ module Gitlab
         @parallel_diff_lines ||= Gitlab::Diff::ParallelDiff.new(self).parallelize
       end
 
-      def mode_changed?
-        a_mode && b_mode && a_mode != b_mode
-      end
-
       def raw_diff
         diff.diff.to_s
       end
@@ -117,20 +150,8 @@ module Gitlab
         diff_lines.count(&:removed?)
       end
 
-      def old_blob(commit = old_content_commit)
-        return unless commit
-
-        repository.blob_at(commit.id, old_path)
-      end
-
-      def blob(commit = content_commit)
-        return unless commit
-
-        repository.blob_at(commit.id, file_path)
-      end
-
       def file_identifier
-        "#{file_path}-#{new_file}-#{deleted_file}-#{renamed_file}"
+        "#{file_path}-#{new_file?}-#{deleted_file?}-#{renamed_file?}"
       end
     end
   end

lib/gitlab/diff/file_collection/base.rb

@@ -2,7 +2,7 @@ module Gitlab
   module Diff
     module FileCollection
       class Base
-        attr_reader :project, :diff_options, :diff_view, :diff_refs
+        attr_reader :project, :diff_options, :diff_refs, :fallback_diff_refs
 
         delegate :count, :size, :real_size, to: :diff_files
 
@@ -10,7 +10,7 @@ module Gitlab
           ::Commit.max_diff_options.merge(ignore_whitespace_change: false, no_collapse: false)
         end
 
-        def initialize(diffable, project:, diff_options: nil, diff_refs: nil)
+        def initialize(diffable, project:, diff_options: nil, diff_refs: nil, fallback_diff_refs: nil)
           diff_options = self.class.default_options.merge(diff_options || {})
 
           @diffable = diffable
@@ -18,6 +18,7 @@ module Gitlab
           @project = project
           @diff_options = diff_options
           @diff_refs = diff_refs
+          @fallback_diff_refs = fallback_diff_refs
         end
 
         def diff_files
@@ -35,7 +36,7 @@ module Gitlab
         private
 
         def decorate_diff!(diff)
-          Gitlab::Diff::File.new(diff, repository: project.repository, diff_refs: diff_refs)
+          Gitlab::Diff::File.new(diff, repository: project.repository, diff_refs: diff_refs, fallback_diff_refs: fallback_diff_refs)
         end
       end
     end

lib/gitlab/diff/file_collection/merge_request_diff.rb

@@ -8,7 +8,8 @@ module Gitlab
           super(merge_request_diff,
             project: merge_request_diff.project,
             diff_options: diff_options,
-            diff_refs: merge_request_diff.diff_refs)
+            diff_refs: merge_request_diff.diff_refs,
+            fallback_diff_refs: merge_request_diff.fallback_diff_refs)
         end
 
         def diff_files

lib/gitlab/diff/highlight.rb

@@ -3,7 +3,7 @@ module Gitlab
     class Highlight
       attr_reader :diff_file, :diff_lines, :raw_lines, :repository
 
-      delegate :old_path, :new_path, :old_ref, :new_ref, to: :diff_file, prefix: :diff
+      delegate :old_path, :new_path, :old_sha, :new_sha, to: :diff_file, prefix: :diff
 
       def initialize(diff_lines, repository: nil)
         @repository = repository
@@ -61,12 +61,12 @@ module Gitlab
 
       def old_lines
         return unless diff_file
-        @old_lines ||= Gitlab::Highlight.highlight_lines(self.repository, diff_old_ref, diff_old_path)
+        @old_lines ||= Gitlab::Highlight.highlight_lines(self.repository, diff_old_sha, diff_old_path)
       end
 
       def new_lines
         return unless diff_file
-        @new_lines ||= Gitlab::Highlight.highlight_lines(self.repository, diff_new_ref, diff_new_path)
+        @new_lines ||= Gitlab::Highlight.highlight_lines(self.repository, diff_new_sha, diff_new_path)
       end
     end
   end

lib/gitlab/git/diff.rb

@@ -11,6 +11,10 @@ module Gitlab
       # Stats properties
       attr_accessor :new_file, :renamed_file, :deleted_file
 
+      alias_method :new_file?, :new_file
+      alias_method :deleted_file?, :deleted_file
+      alias_method :renamed_file?, :renamed_file
+
       attr_accessor :too_large
 
       # The maximum size of a diff to display.
@@ -208,6 +212,10 @@ module Gitlab
         hash
       end
 
+      def mode_changed?
+        a_mode && b_mode && a_mode != b_mode
+      end
+
       def submodule?
         a_mode == '160000' || b_mode == '160000'
       end

lib/gitlab/git/diff_collection.rb

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

spec/lib/gitlab/diff/position_spec.rb

@@ -22,7 +22,7 @@ describe Gitlab::Diff::Position, lib: true do
       it "returns the correct diff file" do
         diff_file = subject.diff_file(project.repository)
 
-        expect(diff_file.new_file).to be true
+        expect(diff_file.new_file?).to be true
         expect(diff_file.new_path).to eq(subject.new_path)
         expect(diff_file.diff_refs).to eq(subject.diff_refs)
       end
@@ -314,7 +314,7 @@ describe Gitlab::Diff::Position, lib: true do
       it "returns the correct diff file" do
         diff_file = subject.diff_file(project.repository)
 
-        expect(diff_file.deleted_file).to be true
+        expect(diff_file.deleted_file?).to be true
         expect(diff_file.old_path).to eq(subject.old_path)
         expect(diff_file.diff_refs).to eq(subject.diff_refs)
       end
@@ -356,7 +356,7 @@ describe Gitlab::Diff::Position, lib: true do
       it "returns the correct diff file" do
         diff_file = subject.diff_file(project.repository)
 
-        expect(diff_file.new_file).to be true
+        expect(diff_file.new_file?).to be true
         expect(diff_file.new_path).to eq(subject.new_path)
         expect(diff_file.diff_refs).to eq(subject.diff_refs)
       end

spec/models/diff_note_spec.rb

@@ -145,7 +145,7 @@ describe DiffNote, models: true do
 
       context "when the merge request's diff refs don't match that of the diff note" do
         before do
-          allow(subject.noteable).to receive(:diff_sha_refs).and_return(commit.diff_refs)
+          allow(subject.noteable).to receive(:diff_refs).and_return(commit.diff_refs)
         end
 
         it "returns false" do
@@ -194,7 +194,7 @@ describe DiffNote, models: true do
 
         context "when the note is outdated" do
           before do
-            allow(merge_request).to receive(:diff_sha_refs).and_return(commit.diff_refs)
+            allow(merge_request).to receive(:diff_refs).and_return(commit.diff_refs)
           end
 
           it "uses the DiffPositionUpdateService" do

spec/models/merge_request_spec.rb

@@ -1243,7 +1243,7 @@ describe MergeRequest, models: true do
     end
   end
 
-  describe "#diff_sha_refs" do
+  describe "#diff_refs" do
     context "with diffs" do
       subject { create(:merge_request, :with_diffs) }
 
@@ -1252,7 +1252,7 @@ describe MergeRequest, models: true do
 
         expect_any_instance_of(Repository).not_to receive(:commit)
 
-        subject.diff_sha_refs
+        subject.diff_refs
       end
 
       it "returns expected diff_refs" do
@@ -1262,7 +1262,7 @@ describe MergeRequest, models: true do
           head_sha:  subject.merge_request_diff.head_commit_sha
         )
 
-        expect(subject.diff_sha_refs).to eq(expected_diff_refs)
+        expect(subject.diff_refs).to eq(expected_diff_refs)
       end
     end
   end