Pass fallback_diff_refs to Diff::File instead of using view helpers

app/controllers/projects/compare_controller.rb

@@ -53,7 +53,6 @@ class Projects::CompareController < Projects::ApplicationController
       @commits = @compare.commits
       @start_commit = @compare.start_commit
       @commit = @compare.commit
-      @base_commit = @compare.base_commit
 
       @diffs = @compare.diffs(diff_options)
 

app/controllers/projects/merge_requests_controller.rb

@@ -502,7 +502,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
   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
@@ -569,7 +568,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/diff_helper.rb

@@ -102,30 +102,14 @@ module DiffHelper
     ].join(' ').html_safe
   end
 
-  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))
+    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)
-    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))
+    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)

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

@@ -60,7 +60,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
@@ -96,7 +96,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,21 +309,14 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def diff_refs
-    return unless diff_start_commit || diff_base_commit
-
-    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?
+    if persisted?
       merge_request_diff.diff_refs
     else
-      diff_refs
+      Gitlab::Diff::DiffRefs.new(
+        base_sha:  diff_base_sha,
+        start_sha: diff_start_sha,
+        head_sha:  diff_head_sha
+      )
     end
   end
 
@@ -858,7 +838,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:)

app/models/merge_request_diff.rb

@@ -150,6 +150,24 @@ 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 find 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
+    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/projects/diffs/_content.html.haml

-- 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) }
+- blob = diff_file.blob
 
 .diff-content
   - if diff_file.too_large?
@@ -18,13 +15,13 @@
         %a.click-to-expand
           Click to expand it.
     - elsif diff_file.diff_lines.length > 0
-      = render "projects/diffs/viewers/text", diff_file: diff_file, blob: blob
+      = 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?
         .nothing-here-block File moved
   - elsif blob.image?
-    = render "projects/diffs/viewers/image", diff_file: diff_file, blob: blob, old_blob: old_blob
+    = render "projects/diffs/viewers/image", diff_file: diff_file
   - else
     .nothing-here-block No preview for this file type

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) }
+.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, 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
@@ -18,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.file_path, project)
-        = view_on_environment_button(diff_commit.id, diff_file.file_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
+  = render 'projects/diffs/content', diff_file: diff_file

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

@@ -4,11 +4,12 @@
   %i.fa.diff-toggle-caret.fa-fw
 
 - if diff_file.submodule?
+  - blob = diff_file.blob
   %span
     = icon('archive fw')
 
     %strong.file-title-name
-      = submodule_link(blob, diff_commit.id, diff_file.repository)
+      = submodule_link(blob, diff_file.content_sha, diff_file.repository)
 
     = copy_file_path_button(blob.path)
 - else

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

+- 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)
 
@@ -12,7 +14,7 @@
     .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)) }
+          %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_blob.size)
@@ -24,7 +26,7 @@
           %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)) }
+          %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(blob.size)

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?

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, :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_sha : new_sha)
+        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_sha)
+        sha = old_content_sha
+        @old_content_commit = repository.commit(sha) if sha
       end
 
-      def old_sha
-        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_sha
-        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
@@ -113,19 +150,6 @@ module Gitlab
         diff_lines.count(&:removed?)
       end
 
-      def old_blob(commit = old_content_commit)
-        return unless commit
-        return if new_file?
-
-        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?}"
       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_view, :diff_refs, :fallback_diff_refs
 
         delegate :count, :size, :real_size, to: :diff_files
 
@@ -10,14 +10,15 @@ 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
-          @diffs        = diffable.raw_diffs(diff_options)
-          @project      = project
+          @diffable = diffable
+          @diffs = diffable.raw_diffs(diff_options)
+          @project = project
           @diff_options = diff_options
-          @diff_refs    = diff_refs
+          @diff_refs = diff_refs
+          @fallback_diff_refs = fallback_diff_refs
         end
 
         def diff_files
@@ -27,7 +28,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

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