Ensure that we never assume old_blob or new_blob are nil44257-viewing-a-particular-commit-gives-500-error-error-undefined-method-binary

These can be a `BatchLoader` which is proxying a nil, while not being concrete
nils themselves.

3 files changed, 38 insertions, 18 deletions

diff --git a/changelogs/unreleased/44257-viewing-a-particular-commit-gives-500-error-error-undefined-method-binary.yml b/changelogs/unreleased/44257-viewing-a-particular-commit-gives-500-error-error-undefined-method-binary.yml
new file mode 100644
index 00000000000..934860b95fe
--- /dev/null
+++ b/changelogs/unreleased/44257-viewing-a-particular-commit-gives-500-error-error-undefined-method-binary.yml
@@ -0,0 +1,5 @@
+---
+title: Fix viewing diffs on old merge requests
+merge_request: 17805
+author:
+type: fixed
diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb
index 34b070dd375..014854da55c 100644
--- a/lib/gitlab/diff/file.rb
+++ b/lib/gitlab/diff/file.rb
@@ -27,8 +27,8 @@ module Gitlab
         @fallback_diff_refs = fallback_diff_refs
 
         # Ensure items are collected in the the batch
-        new_blob
-        old_blob
+        new_blob_lazy
+        old_blob_lazy
       end
 
       def position(position_marker, position_type: :text)
@@ -101,25 +101,19 @@ module Gitlab
       end
 
       def new_blob
-        return unless new_content_sha
-
-        Blob.lazy(repository.project, new_content_sha, file_path)
+        new_blob_lazy&.itself
       end
 
       def old_blob
-        return unless old_content_sha
-
-        Blob.lazy(repository.project, old_content_sha, old_path)
+        old_blob_lazy&.itself
       end
 
       def content_sha
         new_content_sha || old_content_sha
       end
 
-      # Use #itself to check the value wrapped by a BatchLoader instance, rather
-      # than if the BatchLoader instance itself is falsey.
       def blob
-        new_blob&.itself || old_blob&.itself
+        new_blob || old_blob
       end
 
       attr_writer :highlighted_diff_lines
@@ -237,17 +231,14 @@ module Gitlab
 
       private
 
-      # The blob instances are instances of BatchLoader, which means calling
-      # &. directly on them won't work. Object#try also won't work, because Blob
-      # doesn't inherit from Object, but from BasicObject (via SimpleDelegator).
+      # We can't use Object#try because Blob doesn't inherit from Object, but
+      # from BasicObject (via SimpleDelegator).
       def try_blobs(meth)
-        old_blob&.itself&.public_send(meth) || new_blob&.itself&.public_send(meth)
+        old_blob&.public_send(meth) || new_blob&.public_send(meth)
       end
 
-      # We can't use #compact for the same reason we can't use &., but calling
-      # #nil? explicitly does work because it is proxied to the blob itself.
       def valid_blobs
-        [old_blob, new_blob].reject(&:nil?)
+        [old_blob, new_blob].compact
       end
 
       def text_position_properties(line)
@@ -262,6 +253,18 @@ module Gitlab
         old_blob && new_blob && old_blob.id != new_blob.id
       end
 
+      def new_blob_lazy
+        return unless new_content_sha
+
+        Blob.lazy(repository.project, new_content_sha, file_path)
+      end
+
+      def old_blob_lazy
+        return unless old_content_sha
+
+        Blob.lazy(repository.project, old_content_sha, old_path)
+      end
+
       def simple_viewer_class
         return DiffViewer::NotDiffable unless diffable?
 
diff --git a/spec/lib/gitlab/diff/file_spec.rb b/spec/lib/gitlab/diff/file_spec.rb
index 9204ea37963..0c2e18c268a 100644
--- a/spec/lib/gitlab/diff/file_spec.rb
+++ b/spec/lib/gitlab/diff/file_spec.rb
@@ -455,5 +455,17 @@ describe Gitlab::Diff::File do
         expect(diff_file.size).to be_zero
       end
     end
+
+    describe '#different_type?' do
+      it 'returns false' do
+        expect(diff_file).not_to be_different_type
+      end
+    end
+
+    describe '#content_changed?' do
+      it 'returns false' do
+        expect(diff_file).not_to be_content_changed
+      end
+    end
   end
 end