Commit 11f8a97f authored by Douwe Maan's avatar Douwe Maan

Merge branch 'fj-fix-lfs-image-comments-diffs' into 'master'

Fix bug commenting LFS images and displaying LFS files

Closes #43783

See merge request gitlab-org/gitlab-ce!23812
parents 4d6fe39a 5a3e6fdf
...@@ -45,6 +45,9 @@ export default { ...@@ -45,6 +45,9 @@ export default {
isTextFile() { isTextFile() {
return this.diffFile.viewer.name === 'text'; return this.diffFile.viewer.name === 'text';
}, },
errorMessage() {
return this.diffFile.viewer.error;
},
diffFileCommentForm() { diffFileCommentForm() {
return this.getCommentFormForDiffFile(this.diffFile.file_hash); return this.getCommentFormForDiffFile(this.diffFile.file_hash);
}, },
...@@ -75,7 +78,7 @@ export default { ...@@ -75,7 +78,7 @@ export default {
<template> <template>
<div class="diff-content"> <div class="diff-content">
<div class="diff-viewer"> <div v-if="!errorMessage" class="diff-viewer">
<template v-if="isTextFile"> <template v-if="isTextFile">
<empty-file-viewer v-if="diffFile.empty" /> <empty-file-viewer v-if="diffFile.empty" />
<inline-diff-view <inline-diff-view
...@@ -129,5 +132,8 @@ export default { ...@@ -129,5 +132,8 @@ export default {
</div> </div>
</diff-viewer> </diff-viewer>
</div> </div>
<div v-else class="diff-viewer">
<div class="nothing-here-block" v-html="errorMessage"></div>
</div>
</div> </div>
</template> </template>
...@@ -260,7 +260,7 @@ class Projects::BlobController < Projects::ApplicationController ...@@ -260,7 +260,7 @@ class Projects::BlobController < Projects::ApplicationController
extension: blob.extension, extension: blob.extension,
size: blob.raw_size, size: blob.raw_size,
mime_type: blob.mime_type, mime_type: blob.mime_type,
binary: blob.raw_binary?, binary: blob.binary?,
simple_viewer: blob.simple_viewer&.class&.partial_name, simple_viewer: blob.simple_viewer&.class&.partial_name,
rich_viewer: blob.rich_viewer&.class&.partial_name, rich_viewer: blob.rich_viewer&.class&.partial_name,
show_viewer_switcher: !!blob.show_viewer_switcher?, show_viewer_switcher: !!blob.show_viewer_switcher?,
......
...@@ -193,7 +193,7 @@ module BlobHelper ...@@ -193,7 +193,7 @@ module BlobHelper
def open_raw_blob_button(blob) def open_raw_blob_button(blob)
return if blob.empty? return if blob.empty?
return if blob.raw_binary? || blob.stored_externally? return if blob.binary? || blob.stored_externally?
title = 'Open raw' title = 'Open raw'
link_to icon('file-code-o'), blob_raw_path, class: 'btn btn-sm has-tooltip', target: '_blank', rel: 'noopener noreferrer', title: title, data: { container: 'body' } link_to icon('file-code-o'), blob_raw_path, class: 'btn btn-sm has-tooltip', target: '_blank', rel: 'noopener noreferrer', title: title, data: { container: 'body' }
......
...@@ -138,30 +138,6 @@ module DiffHelper ...@@ -138,30 +138,6 @@ module DiffHelper
!diff_file.deleted_file? && @merge_request && @merge_request.source_project !diff_file.deleted_file? && @merge_request && @merge_request.source_project
end end
def diff_render_error_reason(viewer)
case viewer.render_error
when :too_large
"it is too large"
when :server_side_but_stored_externally
case viewer.diff_file.external_storage
when :lfs
'it is stored in LFS'
else
'it is stored externally'
end
end
end
def diff_render_error_options(viewer)
diff_file = viewer.diff_file
options = []
blob_url = project_blob_path(@project, tree_join(diff_file.content_sha, diff_file.file_path))
options << link_to('view the blob', blob_url)
options
end
def diff_file_changed_icon(diff_file) def diff_file_changed_icon(diff_file)
if diff_file.deleted_file? if diff_file.deleted_file?
"file-deletion" "file-deletion"
......
...@@ -110,7 +110,7 @@ module SnippetsHelper ...@@ -110,7 +110,7 @@ module SnippetsHelper
def embedded_snippet_raw_button def embedded_snippet_raw_button
blob = @snippet.blob blob = @snippet.blob
return if blob.empty? || blob.raw_binary? || blob.stored_externally? return if blob.empty? || blob.binary? || blob.stored_externally?
snippet_raw_url = if @snippet.is_a?(PersonalSnippet) snippet_raw_url = if @snippet.is_a?(PersonalSnippet)
raw_snippet_url(@snippet) raw_snippet_url(@snippet)
......
...@@ -102,7 +102,7 @@ class Blob < SimpleDelegator ...@@ -102,7 +102,7 @@ class Blob < SimpleDelegator
# If the blob is a text based blob the content is converted to UTF-8 and any # If the blob is a text based blob the content is converted to UTF-8 and any
# invalid byte sequences are replaced. # invalid byte sequences are replaced.
def data def data
if binary? if binary_in_repo?
super super
else else
@data ||= super.encode(Encoding::UTF_8, invalid: :replace, undef: :replace) @data ||= super.encode(Encoding::UTF_8, invalid: :replace, undef: :replace)
...@@ -149,7 +149,7 @@ class Blob < SimpleDelegator ...@@ -149,7 +149,7 @@ class Blob < SimpleDelegator
# an LFS pointer, we assume the file stored in LFS is binary, unless a # an LFS pointer, we assume the file stored in LFS is binary, unless a
# text-based rich blob viewer matched on the file's extension. Otherwise, this # text-based rich blob viewer matched on the file's extension. Otherwise, this
# depends on the type of the blob itself. # depends on the type of the blob itself.
def raw_binary? def binary?
if stored_externally? if stored_externally?
if rich_viewer if rich_viewer
rich_viewer.binary? rich_viewer.binary?
...@@ -161,7 +161,7 @@ class Blob < SimpleDelegator ...@@ -161,7 +161,7 @@ class Blob < SimpleDelegator
true true
end end
else else
binary? binary_in_repo?
end end
end end
...@@ -180,7 +180,7 @@ class Blob < SimpleDelegator ...@@ -180,7 +180,7 @@ class Blob < SimpleDelegator
end end
def readable_text? def readable_text?
text? && !stored_externally? && !truncated? text_in_repo? && !stored_externally? && !truncated?
end end
def simple_viewer def simple_viewer
...@@ -220,7 +220,7 @@ class Blob < SimpleDelegator ...@@ -220,7 +220,7 @@ class Blob < SimpleDelegator
def simple_viewer_class def simple_viewer_class
if empty? if empty?
BlobViewer::Empty BlobViewer::Empty
elsif raw_binary? elsif binary?
BlobViewer::Download BlobViewer::Download
else # text else # text
BlobViewer::Text BlobViewer::Text
......
...@@ -16,7 +16,7 @@ module BlobViewer ...@@ -16,7 +16,7 @@ module BlobViewer
def initialize(blob) def initialize(blob)
@blob = blob @blob = blob
@initially_binary = blob.binary? @initially_binary = blob.binary_in_repo?
end end
def self.partial_path def self.partial_path
...@@ -52,7 +52,7 @@ module BlobViewer ...@@ -52,7 +52,7 @@ module BlobViewer
end end
def self.can_render?(blob, verify_binary: true) def self.can_render?(blob, verify_binary: true)
return false if verify_binary && binary? != blob.binary? return false if verify_binary && binary? != blob.binary_in_repo?
return true if extensions&.include?(blob.extension) return true if extensions&.include?(blob.extension)
return true if file_types&.include?(blob.file_type) return true if file_types&.include?(blob.file_type)
...@@ -72,7 +72,7 @@ module BlobViewer ...@@ -72,7 +72,7 @@ module BlobViewer
end end
def binary_detected_after_load? def binary_detected_after_load?
!@initially_binary && blob.binary? !@initially_binary && blob.binary_in_repo?
end end
# This method is used on the server side to check whether we can attempt to # This method is used on the server side to check whether we can attempt to
......
...@@ -28,7 +28,7 @@ module BlobLike ...@@ -28,7 +28,7 @@ module BlobLike
nil nil
end end
def binary? def binary_in_repo?
false false
end end
......
...@@ -18,7 +18,7 @@ module DiffViewer ...@@ -18,7 +18,7 @@ module DiffViewer
def initialize(diff_file) def initialize(diff_file)
@diff_file = diff_file @diff_file = diff_file
@initially_binary = diff_file.binary? @initially_binary = diff_file.binary_in_repo?
end end
def self.partial_path def self.partial_path
...@@ -48,7 +48,7 @@ module DiffViewer ...@@ -48,7 +48,7 @@ module DiffViewer
def self.can_render_blob?(blob, verify_binary: true) def self.can_render_blob?(blob, verify_binary: true)
return true if blob.nil? return true if blob.nil?
return false if verify_binary && binary? != blob.binary? return false if verify_binary && binary? != blob.binary_in_repo?
return true if extensions&.include?(blob.extension) return true if extensions&.include?(blob.extension)
return true if file_types&.include?(blob.file_type) return true if file_types&.include?(blob.file_type)
...@@ -70,20 +70,49 @@ module DiffViewer ...@@ -70,20 +70,49 @@ module DiffViewer
end end
def binary_detected_after_load? def binary_detected_after_load?
!@initially_binary && diff_file.binary? !@initially_binary && diff_file.binary_in_repo?
end end
# This method is used on the server side to check whether we can attempt to # This method is used on the server side to check whether we can attempt to
# render the diff_file at all. Human-readable error messages are found in the # render the diff_file at all. The human-readable error message can be
# `BlobHelper#diff_render_error_reason` helper. # retrieved by #render_error_message.
def render_error def render_error
if too_large? if too_large?
:too_large :too_large
end end
end end
def render_error_message
return unless render_error
_("This %{viewer} could not be displayed because %{reason}. You can %{options} instead.") %
{
viewer: switcher_title,
reason: render_error_reason,
options: render_error_options.to_sentence(two_words_connector: _(' or '), last_word_connector: _(', or '))
}
end
def prepare! def prepare!
# To be overridden by subclasses # To be overridden by subclasses
end end
private
def render_error_options
options = []
blob_url = Gitlab::Routing.url_helpers.project_blob_path(diff_file.repository.project,
File.join(diff_file.content_sha, diff_file.file_path))
options << ActionController::Base.helpers.link_to(_('view the blob'), blob_url)
options
end
def render_error_reason
if render_error == :too_large
_("it is too large")
end
end
end end
end end
...@@ -9,6 +9,6 @@ module DiffViewer ...@@ -9,6 +9,6 @@ module DiffViewer
self.extensions = UploaderHelper::IMAGE_EXT self.extensions = UploaderHelper::IMAGE_EXT
self.binary = true self.binary = true
self.switcher_icon = 'picture-o' self.switcher_icon = 'picture-o'
self.switcher_title = 'image diff' self.switcher_title = _('image diff')
end end
end end
...@@ -7,7 +7,7 @@ module DiffViewer ...@@ -7,7 +7,7 @@ module DiffViewer
included do included do
self.type = :rich self.type = :rich
self.switcher_icon = 'file-text-o' self.switcher_icon = 'file-text-o'
self.switcher_title = 'rendered diff' self.switcher_title = _('rendered diff')
end end
end end
end end
...@@ -24,5 +24,17 @@ module DiffViewer ...@@ -24,5 +24,17 @@ module DiffViewer
super super
end end
private
def render_error_reason
return super unless render_error == :server_side_but_stored_externally
if diff_file.external_storage == :lfs
_('it is stored in LFS')
else
_('it is stored externally')
end
end
end end
end end
...@@ -7,7 +7,7 @@ module DiffViewer ...@@ -7,7 +7,7 @@ module DiffViewer
included do included do
self.type = :simple self.type = :simple
self.switcher_icon = 'code' self.switcher_icon = 'code'
self.switcher_title = 'source diff' self.switcher_title = _('source diff')
end end
end end
end end
...@@ -4,4 +4,7 @@ class DiffViewerEntity < Grape::Entity ...@@ -4,4 +4,7 @@ class DiffViewerEntity < Grape::Entity
# Partial name refers directly to a Rails feature, let's avoid # Partial name refers directly to a Rails feature, let's avoid
# using this on the frontend. # using this on the frontend.
expose :partial_name, as: :name expose :partial_name, as: :name
expose :error do |diff_viewer|
diff_viewer.render_error_message
end
end end
.nothing-here-block .nothing-here-block
= _("This %{viewer} could not be displayed because %{reason}.") % { viewer: viewer.switcher_title, reason: diff_render_error_reason(viewer) } = viewer.render_error_message.html_safe
You can
= diff_render_error_options(viewer).to_sentence(two_words_connector: ' or ', last_word_connector: ', or ').html_safe
instead.
---
title: Fix bug commenting on LFS images
merge_request: 23812
author:
type: fixed
...@@ -12,7 +12,7 @@ module Gitlab ...@@ -12,7 +12,7 @@ module Gitlab
end end
def viewable? def viewable?
!large? && text? !large? && text_in_repo?
end end
MEGABYTE = 1024 * 1024 MEGABYTE = 1024 * 1024
...@@ -21,7 +21,7 @@ module Gitlab ...@@ -21,7 +21,7 @@ module Gitlab
size.to_i > MEGABYTE size.to_i > MEGABYTE
end end
def binary? def binary_in_repo?
# Large blobs aren't even loaded into memory # Large blobs aren't even loaded into memory
if data.nil? if data.nil?
true true
...@@ -40,8 +40,8 @@ module Gitlab ...@@ -40,8 +40,8 @@ module Gitlab
end end
end end
def text? def text_in_repo?
!binary? !binary_in_repo?
end end
def image? def image?
...@@ -113,7 +113,7 @@ module Gitlab ...@@ -113,7 +113,7 @@ module Gitlab
def content_type def content_type
# rubocop:disable Style/MultilineTernaryOperator # rubocop:disable Style/MultilineTernaryOperator
# rubocop:disable Style/NestedTernaryOperator # rubocop:disable Style/NestedTernaryOperator
@content_type ||= binary_mime_type? || binary? ? mime_type : @content_type ||= binary_mime_type? || binary_in_repo? ? mime_type :
(encoding ? "text/plain; charset=#{encoding.downcase}" : "text/plain") (encoding ? "text/plain; charset=#{encoding.downcase}" : "text/plain")
# rubocop:enable Style/NestedTernaryOperator # rubocop:enable Style/NestedTernaryOperator
# rubocop:enable Style/MultilineTernaryOperator # rubocop:enable Style/MultilineTernaryOperator
......
...@@ -3,6 +3,8 @@ ...@@ -3,6 +3,8 @@
module Gitlab module Gitlab
module Diff module Diff
class File class File
include Gitlab::Utils::StrongMemoize
attr_reader :diff, :repository, :diff_refs, :fallback_diff_refs, :unique_identifier attr_reader :diff, :repository, :diff_refs, :fallback_diff_refs, :unique_identifier
delegate :new_file?, :deleted_file?, :renamed_file?, delegate :new_file?, :deleted_file?, :renamed_file?,
...@@ -232,12 +234,12 @@ module Gitlab ...@@ -232,12 +234,12 @@ module Gitlab
repository.attributes(file_path).fetch('diff') { true } repository.attributes(file_path).fetch('diff') { true }
end end
def binary? def binary_in_repo?
has_binary_notice? || try_blobs(:binary?) has_binary_notice? || try_blobs(:binary_in_repo?)
end end
def text? def text_in_repo?
!binary? !binary_in_repo?
end end
def external_storage_error? def external_storage_error?
...@@ -279,12 +281,16 @@ module Gitlab ...@@ -279,12 +281,16 @@ module Gitlab
valid_blobs.map(&:empty?).all? valid_blobs.map(&:empty?).all?
end end
def raw_binary? def binary?
try_blobs(:raw_binary?) strong_memoize(:is_binary) do
try_blobs(:binary?)
end
end end
def raw_text? def text?
!raw_binary? && !different_type? strong_memoize(:is_text) do
!binary? && !different_type?
end
end end
def simple_viewer def simple_viewer
...@@ -367,19 +373,19 @@ module Gitlab ...@@ -367,19 +373,19 @@ module Gitlab
return DiffViewer::NotDiffable unless diffable? return DiffViewer::NotDiffable unless diffable?
if content_changed? if content_changed?
if raw_text? if text?
DiffViewer::Text DiffViewer::Text
else else
DiffViewer::NoPreview DiffViewer::NoPreview
end end
elsif new_file? elsif new_file?
if raw_text? if text?
DiffViewer::Text DiffViewer::Text
else else
DiffViewer::Added DiffViewer::Added
end end
elsif deleted_file? elsif deleted_file?
if raw_text? if text?
DiffViewer::Text DiffViewer::Text
else else
DiffViewer::Deleted DiffViewer::Deleted
......
...@@ -100,7 +100,7 @@ module Gitlab ...@@ -100,7 +100,7 @@ module Gitlab
@loaded_all_data = @loaded_size == size @loaded_all_data = @loaded_size == size
end end
def binary? def binary_in_repo?
@binary.nil? ? super : @binary == true @binary.nil? ? super : @binary == true
end end
...@@ -174,7 +174,7 @@ module Gitlab ...@@ -174,7 +174,7 @@ module Gitlab
private private
def has_lfs_version_key? def has_lfs_version_key?
!empty? && text? && data.start_with?("version https://git-lfs.github.com/spec") !empty? && text_in_repo? && data.start_with?("version https://git-lfs.github.com/spec")
end end
end end
end end
......
...@@ -19,6 +19,9 @@ msgstr "" ...@@ -19,6 +19,9 @@ msgstr ""
msgid " Status" msgid " Status"
msgstr "" msgstr ""
msgid " or "
msgstr ""
msgid "%d addition" msgid "%d addition"
msgid_plural "%d additions" msgid_plural "%d additions"
msgstr[0] "" msgstr[0] ""
...@@ -185,6 +188,9 @@ msgstr "" ...@@ -185,6 +188,9 @@ msgstr ""
msgid "+ %{moreCount} more" msgid "+ %{moreCount} more"
msgstr "" msgstr ""
msgid ", or "
msgstr ""
msgid "- Runner is active and can process any new jobs" msgid "- Runner is active and can process any new jobs"
msgstr "" msgstr ""
...@@ -6698,7 +6704,7 @@ msgstr "" ...@@ -6698,7 +6704,7 @@ msgstr ""
msgid "Third party offers" msgid "Third party offers"
msgstr "" msgstr ""
msgid "This %{viewer} could not be displayed because %{reason}." msgid "This %{viewer} could not be displayed because %{reason}. You can %{options} instead."
msgstr "" msgstr ""
msgid "This GitLab instance does not provide any shared Runners yet. Instance administrators can register shared Runners in the admin area." msgid "This GitLab instance does not provide any shared Runners yet. Instance administrators can register shared Runners in the admin area."
...@@ -7891,6 +7897,9 @@ msgstr "" ...@@ -7891,6 +7897,9 @@ msgstr ""
msgid "https://your-bitbucket-server" msgid "https://your-bitbucket-server"
msgstr "" msgstr ""
msgid "image diff"
msgstr ""
msgid "import flow" msgid "import flow"
msgstr "" msgstr ""
...@@ -7903,6 +7912,15 @@ msgstr "" ...@@ -7903,6 +7912,15 @@ msgstr ""
msgid "issue boards" msgid "issue boards"
msgstr "" msgstr ""
msgid "it is stored externally"
msgstr ""
msgid "it is stored in LFS"
msgstr ""
msgid "it is too large"
msgstr ""
msgid "latest deployment" msgid "latest deployment"
msgstr "" msgstr ""
...@@ -8143,6 +8161,9 @@ msgstr "" ...@@ -8143,6 +8161,9 @@ msgstr ""
msgid "remove due date" msgid "remove due date"
msgstr "" msgstr ""
msgid "rendered diff"
msgstr ""
msgid "reply" msgid "reply"
msgid_plural "replies" msgid_plural "replies"
msgstr[0] "" msgstr[0] ""
...@@ -8154,6 +8175,9 @@ msgstr "" ...@@ -8154,6 +8175,9 @@ msgstr ""
msgid "source" msgid "source"
msgstr "" msgstr ""
msgid "source diff"
msgstr ""
msgid "spendCommand|%{slash_command} will update the sum of the time spent." msgid "spendCommand|%{slash_command} will update the sum of the time spent."
msgstr "" msgstr ""
...@@ -8175,6 +8199,9 @@ msgstr "" ...@@ -8175,6 +8199,9 @@ msgstr ""
msgid "view it on GitLab" msgid "view it on GitLab"
msgstr "" msgstr ""
msgid "view the blob"
msgstr ""
msgid "with %{additions} additions, %{deletions} deletions." msgid "with %{additions} additions, %{deletions} deletions."
msgstr "" msgstr ""
......
...@@ -90,9 +90,6 @@ describe 'Merge request > User creates image diff notes', :js do ...@@ -90,9 +90,6 @@ describe 'Merge request > User creates image diff notes', :js do
%w(inline parallel).each do |view| %w(inline parallel).each do |view|
context "#{view} view" do context "#{view} view" do
let(:merge_request) { create(:merge_request_with_diffs, :with_image_diffs, source_project: project, author: user) }
let(:path) { "files/images/ee_repo_logo.png" }
let(:position) do let(:position) do
Gitlab::Diff::Position.new( Gitlab::Diff::Position.new(
old_path: path, old_path: path,
...@@ -108,9 +105,11 @@ describe 'Merge request > User creates image diff notes', :js do ...@@ -108,9 +105,11 @@ describe 'Merge request > User creates image diff notes', :js do
let!(:note) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: position) } let!(:note) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: position) }
describe 'creating a new diff note' do shared_examples 'creates image diff note' do
before do before do
visit diffs_project_merge_request_path(project, merge_request, view: view) visit diffs_project_merge_request_path(project, merge_request, view: view)
wait_for_requests
create_image_diff_note create_image_diff_note
end end
...@@ -132,6 +131,32 @@ describe 'Merge request > User creates image diff notes', :js do ...@@ -132,6 +131,32 @@ describe 'Merge request > User creates image diff notes', :js do
expect(page).to have_content('image diff test comment') expect(page).to have_content('image diff test comment')
end end
end end
context 'when images are not stored in LFS' do
let(:merge_request) { create(:merge_request_with_diffs, :with_image_diffs, source_project: project, author: user) }
let(:path) { 'files/images/ee_repo_logo.png' }
it_behaves_like 'creates image diff note'
end
context 'when images are stored in LFS' do
let(:merge_request) { create(:merge_request, source_project: project, target_project: project, source_branch: 'png-lfs', target_branch: 'master', author: user) }
let(:path) { 'files/images/logo-black.png' }
before do
allow(Gitlab.config.lfs).to receive(:enabled).and_return(true)
project.update_attribute(:lfs_enabled, true)
end
it 'shows lfs badges' do
visit diffs_project_merge_request_path(project, merge_request, view: view)
wait_for_requests
expect(page.all('.diff-file span.label-lfs', visible: :all)).not_to be_empty
end
it_behaves_like 'creates image diff note'
end
end end
end end
......
...@@ -87,20 +87,6 @@ describe 'Merge request > User sees diff', :js do ...@@ -87,20 +87,6 @@ describe 'Merge request > User sees diff', :js do
let(:current_user) { project.owner } let(:current_user) { project.owner }
let(:branch_name) {"test_branch"} let(:branch_name) {"test_branch"}
def create_file(branch_name, file_name, content)
Files::CreateService.new(
project,
current_user,
start_branch: branch_name,
branch_name: branch_name,
commit_message: "Create file",
file_path: file_name,
file_content: content
).execute
project.commit(branch_name)
end
it 'escapes any HTML special characters in the diff chunk header' do it 'escapes any HTML special characters in the diff chunk header' do
file_content = file_content =
<<~CONTENT <<~CONTENT
...@@ -136,5 +122,61 @@ describe 'Merge request > User sees diff', :js do ...@@ -136,5 +122,61 @@ describe 'Merge request > User sees diff', :js do
expect(page).to have_css(".line[lang='rust'] .k") expect(page).to have_css(".line[lang='rust'] .k")
end end
end end
context 'when file is stored in LFS' do
let(:merge_request) { create(:merge_request, source_project: project) }
let(:current_user) { project.owner }
context 'when LFS is enabled on the project' do
before do
allow(Gitlab.config.lfs).to receive(:enabled).and_return(true)
project.update_attribute(:lfs_enabled, true)
create_file('master', file_name, project.repository.blob_at('master', 'files/lfs/lfs_object.iso').data)
visit diffs_project_merge_request_path(project, merge_request)
end
context 'when file is an image', :js do
let(:file_name) { 'files/lfs/image.png' }
it 'shows an error message' do
expect(page).not_to have_content('could not be displayed because it is stored in LFS')
end
end
context 'when file is not an image' do
let(:file_name) { 'files/lfs/ruby.rb' }
it 'shows an error message' do
expect(page).to have_content('This source diff could not be displayed because it is stored in LFS')
end
end
end
context 'when LFS is not enabled' do
before do
visit diffs_project_merge_request_path(project, merge_request)
end
it 'displays the diff' do
expect(page).to have_content('size 1575078')
end
end
end
def create_file(branch_name, file_name, content)
Files::CreateService.new(
project,
current_user,
start_branch: branch_name,
branch_name: branch_name,
commit_message: "Create file",
file_path: file_name,
file_content: content
).execute
project.commit(branch_name)
end
end end
end end
...@@ -93,7 +93,7 @@ describe 'User browses commits' do ...@@ -93,7 +93,7 @@ describe 'User browses commits' do
it 'shows a blank label' do it 'shows a blank label' do
allow_any_instance_of(Gitlab::Diff::File).to receive(:blob).and_return(nil) allow_any_instance_of(Gitlab::Diff::File).to receive(:blob).and_return(nil)
allow_any_instance_of(Gitlab::Diff::File).to receive(:raw_binary?).and_return(true) allow_any_instance_of(Gitlab::Diff::File).to receive(:binary?).and_return(true)
visit(project_commit_path(project, commit)) visit(project_commit_path(project, commit))
......
{ {
"type": "object", "type": "object",
"required": ["name"], "required": [
"name"
],
"properties": { "properties": {
"name": { "type": ["string"] } "name": {
"type": [
"string"
]
},
"error": {
"type": [
"string",
"null"
]
}
}, },
"additionalProperties": false "additionalProperties": false
} }
...@@ -256,43 +256,6 @@ describe DiffHelper do ...@@ -256,43 +256,6 @@ describe DiffHelper do
end end
end end
context 'viewer related' do
let(:viewer) { diff_file.simple_viewer }
before do
assign(:project, project)
end
describe '#diff_render_error_reason' do
context 'for error :too_large' do
before do
expect(viewer).to receive(:render_error).and_return(:too_large)
end
it 'returns an error message' do
expect(helper.diff_render_error_reason(viewer)).to eq('it is too large')
end
end
context 'for error :server_side_but_stored_externally' do
before do
expect(viewer).to receive(:render_error).and_return(:server_side_but_stored_externally)
expect(diff_file).to receive(:external_storage).and_return(:lfs)
end
it 'returns an error message' do
expect(helper.diff_render_error_reason(viewer)).to eq('it is stored in LFS')
end
end
end
describe '#diff_render_error_options' do
it 'includes a "view the blob" link' do
expect(helper.diff_render_error_options(viewer)).to include(/view the blob/)
end
end
end
context '#diff_file_path_text' do context '#diff_file_path_text' do
it 'returns full path by default' do it 'returns full path by default' do
expect(diff_file_path_text(diff_file)).to eq(diff_file.new_path) expect(diff_file_path_text(diff_file)).to eq(diff_file.new_path)
......
...@@ -53,11 +53,11 @@ describe Gitlab::BlobHelper do ...@@ -53,11 +53,11 @@ describe Gitlab::BlobHelper do
describe '#text?' do describe '#text?' do
it 'returns true' do it 'returns true' do
expect(blob.text?).to be_truthy expect(blob.text_in_repo?).to be_truthy
end end
it 'returns false' do it 'returns false' do
expect(large_blob.text?).to be_falsey expect(large_blob.text_in_repo?).to be_falsey
end end
end end
......
...@@ -310,7 +310,7 @@ describe Gitlab::Diff::File do ...@@ -310,7 +310,7 @@ describe Gitlab::Diff::File do
context 'when the content changed' do context 'when the content changed' do
context 'when the file represented by the diff file is binary' do context 'when the file represented by the diff file is binary' do
before do before do
allow(diff_file).to receive(:raw_binary?).and_return(true) allow(diff_file).to receive(:binary?).and_return(true)
end end
it 'returns a No Preview viewer' do it 'returns a No Preview viewer' do
...@@ -345,7 +345,7 @@ describe Gitlab::Diff::File do ...@@ -345,7 +345,7 @@ describe Gitlab::Diff::File do
context 'when the file represented by the diff file is binary' do context 'when the file represented by the diff file is binary' do
before do before do
allow(diff_file).to receive(:raw_binary?).and_return(true) allow(diff_file).to receive(:binary?).and_return(true)
end end
it 'returns an Added viewer' do it 'returns an Added viewer' do
...@@ -380,7 +380,7 @@ describe Gitlab::Diff::File do ...@@ -380,7 +380,7 @@ describe Gitlab::Diff::File do
context 'when the file represented by the diff file is binary' do context 'when the file represented by the diff file is binary' do
before do before do
allow(diff_file).to receive(:raw_binary?).and_return(true) allow(diff_file).to receive(:binary?).and_return(true)
end end
it 'returns a Deleted viewer' do it 'returns a Deleted viewer' do
...@@ -436,7 +436,7 @@ describe Gitlab::Diff::File do ...@@ -436,7 +436,7 @@ describe Gitlab::Diff::File do
allow(diff_file).to receive(:deleted_file?).and_return(false) allow(diff_file).to receive(:deleted_file?).and_return(false)
allow(diff_file).to receive(:renamed_file?).and_return(false) allow(diff_file).to receive(:renamed_file?).and_return(false)
allow(diff_file).to receive(:mode_changed?).and_return(false) allow(diff_file).to receive(:mode_changed?).and_return(false)
allow(diff_file).to receive(:raw_text?).and_return(false) allow(diff_file).to receive(:text?).and_return(false)
end end
it 'returns a No Preview viewer' do it 'returns a No Preview viewer' do
......
...@@ -185,7 +185,7 @@ describe Gitlab::Diff::LinesUnfolder do ...@@ -185,7 +185,7 @@ describe Gitlab::Diff::LinesUnfolder do
let(:project) { create(:project) } let(:project) { create(:project) }
let(:old_blob) { Gitlab::Git::Blob.new(data: raw_old_blob) } let(:old_blob) { Blob.decorate(Gitlab::Git::Blob.new(data: raw_old_blob, size: 10)) }
let(:diff) do let(:diff) do
Gitlab::Git::Diff.new(diff: raw_diff, Gitlab::Git::Diff.new(diff: raw_diff,
......
...@@ -59,7 +59,7 @@ describe Gitlab::Git::Blob, :seed_helper do ...@@ -59,7 +59,7 @@ describe Gitlab::Git::Blob, :seed_helper do
it { expect(blob.data[0..10]).to eq("*.rbc\n*.sas") } it { expect(blob.data[0..10]).to eq("*.rbc\n*.sas") }
it { expect(blob.size).to eq(241) } it { expect(blob.size).to eq(241) }
it { expect(blob.mode).to eq("100644") } it { expect(blob.mode).to eq("100644") }
it { expect(blob).not_to be_binary } it { expect(blob).not_to be_binary_in_repo }
end end
context 'file in root with leading slash' do context 'file in root with leading slash' do
...@@ -92,7 +92,7 @@ describe Gitlab::Git::Blob, :seed_helper do ...@@ -92,7 +92,7 @@ describe Gitlab::Git::Blob, :seed_helper do
end end
it 'does not mark the blob as binary' do it 'does not mark the blob as binary' do
expect(blob).not_to be_binary expect(blob).not_to be_binary_in_repo
end end
end end
...@@ -123,7 +123,7 @@ describe Gitlab::Git::Blob, :seed_helper do ...@@ -123,7 +123,7 @@ describe Gitlab::Git::Blob, :seed_helper do
.with(hash_including(binary: true)) .with(hash_including(binary: true))
.and_call_original .and_call_original
expect(blob).to be_binary expect(blob).to be_binary_in_repo
end end
end end
end end
...@@ -196,7 +196,7 @@ describe Gitlab::Git::Blob, :seed_helper do ...@@ -196,7 +196,7 @@ describe Gitlab::Git::Blob, :seed_helper do
it { expect(blob.id).to eq('409f37c4f05865e4fb208c771485f211a22c4c2d') } it { expect(blob.id).to eq('409f37c4f05865e4fb208c771485f211a22c4c2d') }
it { expect(blob.data).to eq('') } it { expect(blob.data).to eq('') }
it 'does not mark the blob as binary' do it 'does not mark the blob as binary' do
expect(blob).not_to be_binary expect(blob).not_to be_binary_in_repo
end end
end end
......
...@@ -21,7 +21,7 @@ describe Gitlab::GitalyClient::BlobsStitcher do ...@@ -21,7 +21,7 @@ describe Gitlab::GitalyClient::BlobsStitcher do
expect(blobs[0].size).to eq(1642) expect(blobs[0].size).to eq(1642)
expect(blobs[0].commit_id).to eq('f00ba7') expect(blobs[0].commit_id).to eq('f00ba7')
expect(blobs[0].data).to eq("first-line\nsecond-line") expect(blobs[0].data).to eq("first-line\nsecond-line")
expect(blobs[0].binary?).to be false expect(blobs[0].binary_in_repo?).to be false
expect(blobs[1].id).to eq('abcdef2') expect(blobs[1].id).to eq('abcdef2')
expect(blobs[1].mode).to eq('100644') expect(blobs[1].mode).to eq('100644')
...@@ -30,7 +30,7 @@ describe Gitlab::GitalyClient::BlobsStitcher do ...@@ -30,7 +30,7 @@ describe Gitlab::GitalyClient::BlobsStitcher do
expect(blobs[1].size).to eq(2461) expect(blobs[1].size).to eq(2461)
expect(blobs[1].commit_id).to eq('f00ba8') expect(blobs[1].commit_id).to eq('f00ba8')
expect(blobs[1].data).to eq("GIF87a\x90\x01".b) expect(blobs[1].data).to eq("GIF87a\x90\x01".b)
expect(blobs[1].binary?).to be true expect(blobs[1].binary_in_repo?).to be true
end end
end end
end end
...@@ -122,14 +122,14 @@ describe Blob do ...@@ -122,14 +122,14 @@ describe Blob do
end end
end end
describe '#raw_binary?' do describe '#binary?' do
context 'if the blob is stored externally' do context 'if the blob is stored externally' do
context 'if the extension has a rich viewer' do context 'if the extension has a rich viewer' do
context 'if the viewer is binary' do context 'if the viewer is binary' do
it 'returns true' do it 'returns true' do
blob = fake_blob(path: 'file.pdf', lfs: true) blob = fake_blob(path: 'file.pdf', lfs: true)
expect(blob.raw_binary?).to be_truthy expect(blob.binary?).to be_truthy
end end
end end
...@@ -137,7 +137,7 @@ describe Blob do ...@@ -137,7 +137,7 @@ describe Blob do
it 'return false' do it 'return false' do
blob = fake_blob(path: 'file.md', lfs: true) blob = fake_blob(path: 'file.md', lfs: true)
expect(blob.raw_binary?).to be_falsey expect(blob.binary?).to be_falsey
end end
end end
end end
...@@ -148,7 +148,7 @@ describe Blob do ...@@ -148,7 +148,7 @@ describe Blob do
it 'returns false' do it 'returns false' do
blob = fake_blob(path: 'file.txt', lfs: true) blob = fake_blob(path: 'file.txt', lfs: true)
expect(blob.raw_binary?).to be_falsey expect(blob.binary?).to be_falsey
end end
end end
...@@ -156,7 +156,7 @@ describe Blob do ...@@ -156,7 +156,7 @@ describe Blob do
it 'returns false' do it 'returns false' do
blob = fake_blob(path: 'file.ics', lfs: true) blob = fake_blob(path: 'file.ics', lfs: true)
expect(blob.raw_binary?).to be_falsey expect(blob.binary?).to be_falsey
end end
end end
end end
...@@ -166,7 +166,7 @@ describe Blob do ...@@ -166,7 +166,7 @@ describe Blob do
it 'returns false' do it 'returns false' do
blob = fake_blob(path: 'file.rb', lfs: true) blob = fake_blob(path: 'file.rb', lfs: true)
expect(blob.raw_binary?).to be_falsey expect(blob.binary?).to be_falsey
end end
end end
...@@ -174,7 +174,7 @@ describe Blob do ...@@ -174,7 +174,7 @@ describe Blob do
it 'returns true' do it 'returns true' do
blob = fake_blob(path: 'file.exe', lfs: true) blob = fake_blob(path: 'file.exe', lfs: true)
expect(blob.raw_binary?).to be_truthy expect(blob.binary?).to be_truthy
end end
end end
end end
...@@ -184,7 +184,7 @@ describe Blob do ...@@ -184,7 +184,7 @@ describe Blob do
it 'returns false' do it 'returns false' do
blob = fake_blob(path: 'file.ini', lfs: true) blob = fake_blob(path: 'file.ini', lfs: true)
expect(blob.raw_binary?).to be_falsey expect(blob.binary?).to be_falsey
end end
end end
...@@ -192,7 +192,7 @@ describe Blob do ...@@ -192,7 +192,7 @@ describe Blob do
it 'returns true' do it 'returns true' do
blob = fake_blob(path: 'file.wtf', lfs: true) blob = fake_blob(path: 'file.wtf', lfs: true)
expect(blob.raw_binary?).to be_truthy expect(blob.binary?).to be_truthy
end end
end end
end end
...@@ -204,7 +204,7 @@ describe Blob do ...@@ -204,7 +204,7 @@ describe Blob do
it 'returns true' do it 'returns true' do
blob = fake_blob(path: 'file.pdf', binary: true) blob = fake_blob(path: 'file.pdf', binary: true)
expect(blob.raw_binary?).to be_truthy expect(blob.binary?).to be_truthy
end end
end end
...@@ -212,7 +212,7 @@ describe Blob do ...@@ -212,7 +212,7 @@ describe Blob do
it 'return false' do it 'return false' do
blob = fake_blob(path: 'file.md') blob = fake_blob(path: 'file.md')
expect(blob.raw_binary?).to be_falsey expect(blob.binary?).to be_falsey
end end
end end
end end
......
...@@ -58,7 +58,7 @@ describe DiffViewer::Base do ...@@ -58,7 +58,7 @@ describe DiffViewer::Base do
context 'when the binaryness does not match' do context 'when the binaryness does not match' do
before do before do
allow_any_instance_of(Blob).to receive(:binary?).and_return(true) allow_any_instance_of(Blob).to receive(:binary_in_repo?).and_return(true)
end end
it 'returns false' do it 'returns false' do
...@@ -141,4 +141,25 @@ describe DiffViewer::Base do ...@@ -141,4 +141,25 @@ describe DiffViewer::Base do
end end
end end
end end
describe '#render_error_message' do
it 'returns nothing when no render_error' do
expect(viewer.render_error).to be_nil
expect(viewer.render_error_message).to be_nil
end
context 'when render_error error' do
before do
allow(viewer).to receive(:render_error).and_return(:too_large)
end
it 'returns an error message' do
expect(viewer.render_error_message).to include('it is too large')
end
it 'includes a "view the blob" link' do
expect(viewer.render_error_message).to include('view the blob')
end
end
end
end end
...@@ -32,4 +32,24 @@ describe DiffViewer::ServerSide do ...@@ -32,4 +32,24 @@ describe DiffViewer::ServerSide do
end end
end end
end end
describe '#render_error_reason' do
context 'when the diff file is stored externally' do
before do
allow(diff_file).to receive(:stored_externally?).and_return(true)
end
it 'returns error message if stored in LFS' do
allow(diff_file).to receive(:external_storage).and_return(:lfs)
expect(subject.render_error_message).to include('it is stored in LFS')
end
it 'returns error message if stored externally' do
allow(diff_file).to receive(:external_storage).and_return(:foo)
expect(subject.render_error_message).to include('it is stored externally')
end
end
end
end end
...@@ -23,7 +23,7 @@ module FakeBlobHelpers ...@@ -23,7 +23,7 @@ module FakeBlobHelpers
0 0
end end
def binary? def binary_in_repo?
@binary @binary
end end
......
...@@ -62,7 +62,8 @@ module TestEnv ...@@ -62,7 +62,8 @@ module TestEnv
'between-create-delete-modify-move' => '3f5f443', 'between-create-delete-modify-move' => '3f5f443',
'after-create-delete-modify-move' => 'ba3faa7', 'after-create-delete-modify-move' => 'ba3faa7',
'with-codeowners' => '219560e', 'with-codeowners' => '219560e',
'submodule_inside_folder' => 'b491b92' 'submodule_inside_folder' => 'b491b92',
'png-lfs' => 'fe42f41'
}.freeze }.freeze
# gitlab-test-fork is a fork of gitlab-fork, but we don't necessarily # gitlab-test-fork is a fork of gitlab-fork, but we don't necessarily
......
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