Commit 528b5eeb authored by Sean McGivern's avatar Sean McGivern

Fix error when viewing diffs without blobs

Old merge requests can have diffs without corresponding blobs. (This also may be
possible for commit diffs in corrupt repositories.)

We can't use the `&.` operator on the blobs, because the blob objects are never
nil, but `BatchLoader` instances that delegate to `Blob`. We can't use
`Object#try`, because `Blob` doesn't inherit from `Object`.

`BatchLoader` provides a `__sync` method that returns the delegated object, but
using `itself` also works because it's forwarded, and will work for
non-`BatchLoader` instances too. So the simplest solution is to just use that
with the `&.` operator.
parent e5a9b9a1
---
title: Fix viewing merge request diffs where the underlying blobs are unavailable
merge_request:
author:
type: fixed
...@@ -116,8 +116,10 @@ module Gitlab ...@@ -116,8 +116,10 @@ module Gitlab
new_content_sha || old_content_sha new_content_sha || old_content_sha
end end
# Use #itself to check the value wrapped by a BatchLoader instance, rather
# than if the BatchLoader instance itself is falsey.
def blob def blob
new_blob || old_blob new_blob&.itself || old_blob&.itself
end end
attr_writer :highlighted_diff_lines attr_writer :highlighted_diff_lines
...@@ -173,7 +175,7 @@ module Gitlab ...@@ -173,7 +175,7 @@ module Gitlab
end end
def binary? def binary?
has_binary_notice? || old_blob&.binary? || new_blob&.binary? has_binary_notice? || try_blobs(:binary?)
end end
def text? def text?
...@@ -181,15 +183,15 @@ module Gitlab ...@@ -181,15 +183,15 @@ module Gitlab
end end
def external_storage_error? def external_storage_error?
old_blob&.external_storage_error? || new_blob&.external_storage_error? try_blobs(:external_storage_error?)
end end
def stored_externally? def stored_externally?
old_blob&.stored_externally? || new_blob&.stored_externally? try_blobs(:stored_externally?)
end end
def external_storage def external_storage
old_blob&.external_storage || new_blob&.external_storage try_blobs(:external_storage)
end end
def content_changed? def content_changed?
...@@ -204,15 +206,15 @@ module Gitlab ...@@ -204,15 +206,15 @@ module Gitlab
end end
def size def size
[old_blob&.size, new_blob&.size].compact.sum valid_blobs.map(&:size).sum
end end
def raw_size def raw_size
[old_blob&.raw_size, new_blob&.raw_size].compact.sum valid_blobs.map(&:raw_size).sum
end end
def raw_binary? def raw_binary?
old_blob&.raw_binary? || new_blob&.raw_binary? try_blobs(:raw_binary?)
end end
def raw_text? def raw_text?
...@@ -235,6 +237,19 @@ module Gitlab ...@@ -235,6 +237,19 @@ module Gitlab
private 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).
def try_blobs(meth)
old_blob&.itself&.public_send(meth) || new_blob&.itself&.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?)
end
def text_position_properties(line) def text_position_properties(line)
{ old_line: line.old_line, new_line: line.new_line } { old_line: line.old_line, new_line: line.new_line }
end end
......
...@@ -431,4 +431,29 @@ describe Gitlab::Diff::File do ...@@ -431,4 +431,29 @@ describe Gitlab::Diff::File do
end end
end end
end end
context 'when neither blob exists' do
let(:blank_diff_refs) { Gitlab::Diff::DiffRefs.new(base_sha: Gitlab::Git::BLANK_SHA, head_sha: Gitlab::Git::BLANK_SHA) }
let(:diff_file) { described_class.new(diff, diff_refs: blank_diff_refs, repository: project.repository) }
describe '#blob' do
it 'returns a concrete nil so it can be used in boolean expressions' do
actual = diff_file.blob && true
expect(actual).to be_nil
end
end
describe '#binary?' do
it 'returns false' do
expect(diff_file).not_to be_binary
end
end
describe '#size' do
it 'returns zero' do
expect(diff_file.size).to be_zero
end
end
end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment