summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--app/assets/javascripts/diffs/components/diff_content.vue8
-rw-r--r--app/controllers/projects/blob_controller.rb2
-rw-r--r--app/helpers/blob_helper.rb2
-rw-r--r--app/helpers/diff_helper.rb24
-rw-r--r--app/helpers/snippets_helper.rb2
-rw-r--r--app/models/blob.rb10
-rw-r--r--app/models/blob_viewer/base.rb6
-rw-r--r--app/models/concerns/blob_like.rb2
-rw-r--r--app/models/diff_viewer/base.rb39
-rw-r--r--app/models/diff_viewer/image.rb2
-rw-r--r--app/models/diff_viewer/rich.rb2
-rw-r--r--app/models/diff_viewer/server_side.rb12
-rw-r--r--app/models/diff_viewer/simple.rb2
-rw-r--r--app/serializers/diff_viewer_entity.rb3
-rw-r--r--app/views/projects/diffs/_render_error.html.haml6
-rw-r--r--changelogs/unreleased/fj-fix-lfs-image-comments-diffs.yml5
-rw-r--r--lib/gitlab/blob_helper.rb10
-rw-r--r--lib/gitlab/diff/file.rb28
-rw-r--r--lib/gitlab/git/blob.rb4
-rw-r--r--locale/gitlab.pot29
-rw-r--r--spec/features/merge_request/user_creates_image_diff_notes_spec.rb33
-rw-r--r--spec/features/merge_request/user_sees_diff_spec.rb70
-rw-r--r--spec/features/projects/commits/user_browses_commits_spec.rb2
-rw-r--r--spec/fixtures/api/schemas/entities/diff_viewer.json16
-rw-r--r--spec/helpers/diff_helper_spec.rb37
-rw-r--r--spec/lib/gitlab/blob_helper_spec.rb4
-rw-r--r--spec/lib/gitlab/diff/file_spec.rb8
-rw-r--r--spec/lib/gitlab/diff/lines_unfolder_spec.rb2
-rw-r--r--spec/lib/gitlab/git/blob_spec.rb8
-rw-r--r--spec/lib/gitlab/gitaly_client/blobs_stitcher_spec.rb4
-rw-r--r--spec/models/blob_spec.rb22
-rw-r--r--spec/models/diff_viewer/base_spec.rb23
-rw-r--r--spec/models/diff_viewer/server_side_spec.rb20
-rw-r--r--spec/support/helpers/fake_blob_helpers.rb2
-rw-r--r--spec/support/helpers/test_env.rb3
35 files changed, 298 insertions, 154 deletions
diff --git a/app/assets/javascripts/diffs/components/diff_content.vue b/app/assets/javascripts/diffs/components/diff_content.vue
index 42d09e44768..ba6dcd63880 100644
--- a/app/assets/javascripts/diffs/components/diff_content.vue
+++ b/app/assets/javascripts/diffs/components/diff_content.vue
@@ -45,6 +45,9 @@ export default {
isTextFile() {
return this.diffFile.viewer.name === 'text';
},
+ errorMessage() {
+ return this.diffFile.viewer.error;
+ },
diffFileCommentForm() {
return this.getCommentFormForDiffFile(this.diffFile.file_hash);
},
@@ -75,7 +78,7 @@ export default {
<template>
<div class="diff-content">
- <div class="diff-viewer">
+ <div v-if="!errorMessage" class="diff-viewer">
<template v-if="isTextFile">
<empty-file-viewer v-if="diffFile.empty" />
<inline-diff-view
@@ -129,5 +132,8 @@ export default {
</div>
</diff-viewer>
</div>
+ <div v-else class="diff-viewer">
+ <div class="nothing-here-block" v-html="errorMessage"></div>
+ </div>
</div>
</template>
diff --git a/app/controllers/projects/blob_controller.rb b/app/controllers/projects/blob_controller.rb
index 60fabd15333..ff286c0ccf0 100644
--- a/app/controllers/projects/blob_controller.rb
+++ b/app/controllers/projects/blob_controller.rb
@@ -260,7 +260,7 @@ class Projects::BlobController < Projects::ApplicationController
extension: blob.extension,
size: blob.raw_size,
mime_type: blob.mime_type,
- binary: blob.raw_binary?,
+ binary: blob.binary?,
simple_viewer: blob.simple_viewer&.class&.partial_name,
rich_viewer: blob.rich_viewer&.class&.partial_name,
show_viewer_switcher: !!blob.show_viewer_switcher?,
diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb
index 4c8e1b209c0..3dea0975beb 100644
--- a/app/helpers/blob_helper.rb
+++ b/app/helpers/blob_helper.rb
@@ -193,7 +193,7 @@ module BlobHelper
def open_raw_blob_button(blob)
return if blob.empty?
- return if blob.raw_binary? || blob.stored_externally?
+ return if blob.binary? || blob.stored_externally?
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' }
diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb
index b6844d36052..32431959851 100644
--- a/app/helpers/diff_helper.rb
+++ b/app/helpers/diff_helper.rb
@@ -138,30 +138,6 @@ module DiffHelper
!diff_file.deleted_file? && @merge_request && @merge_request.source_project
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)
if diff_file.deleted_file?
"file-deletion"
diff --git a/app/helpers/snippets_helper.rb b/app/helpers/snippets_helper.rb
index c7d31f3469d..8fded0efe4d 100644
--- a/app/helpers/snippets_helper.rb
+++ b/app/helpers/snippets_helper.rb
@@ -110,7 +110,7 @@ module SnippetsHelper
def embedded_snippet_raw_button
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)
raw_snippet_url(@snippet)
diff --git a/app/models/blob.rb b/app/models/blob.rb
index 66a0925c495..c5766eb0327 100644
--- a/app/models/blob.rb
+++ b/app/models/blob.rb
@@ -102,7 +102,7 @@ class Blob < SimpleDelegator
# If the blob is a text based blob the content is converted to UTF-8 and any
# invalid byte sequences are replaced.
def data
- if binary?
+ if binary_in_repo?
super
else
@data ||= super.encode(Encoding::UTF_8, invalid: :replace, undef: :replace)
@@ -149,7 +149,7 @@ class Blob < SimpleDelegator
# 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
# depends on the type of the blob itself.
- def raw_binary?
+ def binary?
if stored_externally?
if rich_viewer
rich_viewer.binary?
@@ -161,7 +161,7 @@ class Blob < SimpleDelegator
true
end
else
- binary?
+ binary_in_repo?
end
end
@@ -180,7 +180,7 @@ class Blob < SimpleDelegator
end
def readable_text?
- text? && !stored_externally? && !truncated?
+ text_in_repo? && !stored_externally? && !truncated?
end
def simple_viewer
@@ -220,7 +220,7 @@ class Blob < SimpleDelegator
def simple_viewer_class
if empty?
BlobViewer::Empty
- elsif raw_binary?
+ elsif binary?
BlobViewer::Download
else # text
BlobViewer::Text
diff --git a/app/models/blob_viewer/base.rb b/app/models/blob_viewer/base.rb
index eaaf9af1330..df6b9bb2f0b 100644
--- a/app/models/blob_viewer/base.rb
+++ b/app/models/blob_viewer/base.rb
@@ -16,7 +16,7 @@ module BlobViewer
def initialize(blob)
@blob = blob
- @initially_binary = blob.binary?
+ @initially_binary = blob.binary_in_repo?
end
def self.partial_path
@@ -52,7 +52,7 @@ module BlobViewer
end
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 file_types&.include?(blob.file_type)
@@ -72,7 +72,7 @@ module BlobViewer
end
def binary_detected_after_load?
- !@initially_binary && blob.binary?
+ !@initially_binary && blob.binary_in_repo?
end
# This method is used on the server side to check whether we can attempt to
diff --git a/app/models/concerns/blob_like.rb b/app/models/concerns/blob_like.rb
index f20f01486a5..dc80f8d62f4 100644
--- a/app/models/concerns/blob_like.rb
+++ b/app/models/concerns/blob_like.rb
@@ -28,7 +28,7 @@ module BlobLike
nil
end
- def binary?
+ def binary_in_repo?
false
end
diff --git a/app/models/diff_viewer/base.rb b/app/models/diff_viewer/base.rb
index 1176861a827..527ee33b83b 100644
--- a/app/models/diff_viewer/base.rb
+++ b/app/models/diff_viewer/base.rb
@@ -18,7 +18,7 @@ module DiffViewer
def initialize(diff_file)
@diff_file = diff_file
- @initially_binary = diff_file.binary?
+ @initially_binary = diff_file.binary_in_repo?
end
def self.partial_path
@@ -48,7 +48,7 @@ module DiffViewer
def self.can_render_blob?(blob, verify_binary: true)
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 file_types&.include?(blob.file_type)
@@ -70,20 +70,49 @@ module DiffViewer
end
def binary_detected_after_load?
- !@initially_binary && diff_file.binary?
+ !@initially_binary && diff_file.binary_in_repo?
end
# 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
- # `BlobHelper#diff_render_error_reason` helper.
+ # render the diff_file at all. The human-readable error message can be
+ # retrieved by #render_error_message.
def render_error
if too_large?
:too_large
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!
# To be overridden by subclasses
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
diff --git a/app/models/diff_viewer/image.rb b/app/models/diff_viewer/image.rb
index c356c2ca50e..350bef1d42a 100644
--- a/app/models/diff_viewer/image.rb
+++ b/app/models/diff_viewer/image.rb
@@ -9,6 +9,6 @@ module DiffViewer
self.extensions = UploaderHelper::IMAGE_EXT
self.binary = true
self.switcher_icon = 'picture-o'
- self.switcher_title = 'image diff'
+ self.switcher_title = _('image diff')
end
end
diff --git a/app/models/diff_viewer/rich.rb b/app/models/diff_viewer/rich.rb
index 2faa1be6567..5caefa2031c 100644
--- a/app/models/diff_viewer/rich.rb
+++ b/app/models/diff_viewer/rich.rb
@@ -7,7 +7,7 @@ module DiffViewer
included do
self.type = :rich
self.switcher_icon = 'file-text-o'
- self.switcher_title = 'rendered diff'
+ self.switcher_title = _('rendered diff')
end
end
end
diff --git a/app/models/diff_viewer/server_side.rb b/app/models/diff_viewer/server_side.rb
index 977204e6c97..0877c9dddec 100644
--- a/app/models/diff_viewer/server_side.rb
+++ b/app/models/diff_viewer/server_side.rb
@@ -24,5 +24,17 @@ module DiffViewer
super
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
diff --git a/app/models/diff_viewer/simple.rb b/app/models/diff_viewer/simple.rb
index 8d28ca5239a..929d8ad5a7e 100644
--- a/app/models/diff_viewer/simple.rb
+++ b/app/models/diff_viewer/simple.rb
@@ -7,7 +7,7 @@ module DiffViewer
included do
self.type = :simple
self.switcher_icon = 'code'
- self.switcher_title = 'source diff'
+ self.switcher_title = _('source diff')
end
end
end
diff --git a/app/serializers/diff_viewer_entity.rb b/app/serializers/diff_viewer_entity.rb
index 27fba03cb3f..587fa2347fd 100644
--- a/app/serializers/diff_viewer_entity.rb
+++ b/app/serializers/diff_viewer_entity.rb
@@ -4,4 +4,7 @@ class DiffViewerEntity < Grape::Entity
# Partial name refers directly to a Rails feature, let's avoid
# using this on the frontend.
expose :partial_name, as: :name
+ expose :error do |diff_viewer|
+ diff_viewer.render_error_message
+ end
end
diff --git a/app/views/projects/diffs/_render_error.html.haml b/app/views/projects/diffs/_render_error.html.haml
index c3dc47a56a7..7dbd9897e83 100644
--- a/app/views/projects/diffs/_render_error.html.haml
+++ b/app/views/projects/diffs/_render_error.html.haml
@@ -1,6 +1,2 @@
.nothing-here-block
- = _("This %{viewer} could not be displayed because %{reason}.") % { viewer: viewer.switcher_title, reason: diff_render_error_reason(viewer) }
-
- You can
- = diff_render_error_options(viewer).to_sentence(two_words_connector: ' or ', last_word_connector: ', or ').html_safe
- instead.
+ = viewer.render_error_message.html_safe
diff --git a/changelogs/unreleased/fj-fix-lfs-image-comments-diffs.yml b/changelogs/unreleased/fj-fix-lfs-image-comments-diffs.yml
new file mode 100644
index 00000000000..dc1fe5d7417
--- /dev/null
+++ b/changelogs/unreleased/fj-fix-lfs-image-comments-diffs.yml
@@ -0,0 +1,5 @@
+---
+title: Fix bug commenting on LFS images
+merge_request: 23812
+author:
+type: fixed
diff --git a/lib/gitlab/blob_helper.rb b/lib/gitlab/blob_helper.rb
index 488c1d85387..d3e15a79a8b 100644
--- a/lib/gitlab/blob_helper.rb
+++ b/lib/gitlab/blob_helper.rb
@@ -12,7 +12,7 @@ module Gitlab
end
def viewable?
- !large? && text?
+ !large? && text_in_repo?
end
MEGABYTE = 1024 * 1024
@@ -21,7 +21,7 @@ module Gitlab
size.to_i > MEGABYTE
end
- def binary?
+ def binary_in_repo?
# Large blobs aren't even loaded into memory
if data.nil?
true
@@ -40,8 +40,8 @@ module Gitlab
end
end
- def text?
- !binary?
+ def text_in_repo?
+ !binary_in_repo?
end
def image?
@@ -113,7 +113,7 @@ module Gitlab
def content_type
# rubocop:disable Style/MultilineTernaryOperator
# 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")
# rubocop:enable Style/NestedTernaryOperator
# rubocop:enable Style/MultilineTernaryOperator
diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb
index bcbded6be9a..e410d5a8333 100644
--- a/lib/gitlab/diff/file.rb
+++ b/lib/gitlab/diff/file.rb
@@ -3,6 +3,8 @@
module Gitlab
module Diff
class File
+ include Gitlab::Utils::StrongMemoize
+
attr_reader :diff, :repository, :diff_refs, :fallback_diff_refs, :unique_identifier
delegate :new_file?, :deleted_file?, :renamed_file?,
@@ -232,12 +234,12 @@ module Gitlab
repository.attributes(file_path).fetch('diff') { true }
end
- def binary?
- has_binary_notice? || try_blobs(:binary?)
+ def binary_in_repo?
+ has_binary_notice? || try_blobs(:binary_in_repo?)
end
- def text?
- !binary?
+ def text_in_repo?
+ !binary_in_repo?
end
def external_storage_error?
@@ -279,12 +281,16 @@ module Gitlab
valid_blobs.map(&:empty?).all?
end
- def raw_binary?
- try_blobs(:raw_binary?)
+ def binary?
+ strong_memoize(:is_binary) do
+ try_blobs(:binary?)
+ end
end
- def raw_text?
- !raw_binary? && !different_type?
+ def text?
+ strong_memoize(:is_text) do
+ !binary? && !different_type?
+ end
end
def simple_viewer
@@ -367,19 +373,19 @@ module Gitlab
return DiffViewer::NotDiffable unless diffable?
if content_changed?
- if raw_text?
+ if text?
DiffViewer::Text
else
DiffViewer::NoPreview
end
elsif new_file?
- if raw_text?
+ if text?
DiffViewer::Text
else
DiffViewer::Added
end
elsif deleted_file?
- if raw_text?
+ if text?
DiffViewer::Text
else
DiffViewer::Deleted
diff --git a/lib/gitlab/git/blob.rb b/lib/gitlab/git/blob.rb
index 2d25389594e..259a2b7911a 100644
--- a/lib/gitlab/git/blob.rb
+++ b/lib/gitlab/git/blob.rb
@@ -100,7 +100,7 @@ module Gitlab
@loaded_all_data = @loaded_size == size
end
- def binary?
+ def binary_in_repo?
@binary.nil? ? super : @binary == true
end
@@ -174,7 +174,7 @@ module Gitlab
private
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
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index b27efe04966..bd0f2b5d122 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -19,6 +19,9 @@ msgstr ""
msgid " Status"
msgstr ""
+msgid " or "
+msgstr ""
+
msgid "%d addition"
msgid_plural "%d additions"
msgstr[0] ""
@@ -185,6 +188,9 @@ msgstr ""
msgid "+ %{moreCount} more"
msgstr ""
+msgid ", or "
+msgstr ""
+
msgid "- Runner is active and can process any new jobs"
msgstr ""
@@ -6698,7 +6704,7 @@ msgstr ""
msgid "Third party offers"
msgstr ""
-msgid "This %{viewer} could not be displayed because %{reason}."
+msgid "This %{viewer} could not be displayed because %{reason}. You can %{options} instead."
msgstr ""
msgid "This GitLab instance does not provide any shared Runners yet. Instance administrators can register shared Runners in the admin area."
@@ -7888,6 +7894,9 @@ msgstr ""
msgid "https://your-bitbucket-server"
msgstr ""
+msgid "image diff"
+msgstr ""
+
msgid "import flow"
msgstr ""
@@ -7900,6 +7909,15 @@ msgstr ""
msgid "issue boards"
msgstr ""
+msgid "it is stored externally"
+msgstr ""
+
+msgid "it is stored in LFS"
+msgstr ""
+
+msgid "it is too large"
+msgstr ""
+
msgid "latest deployment"
msgstr ""
@@ -8140,6 +8158,9 @@ msgstr ""
msgid "remove due date"
msgstr ""
+msgid "rendered diff"
+msgstr ""
+
msgid "reply"
msgid_plural "replies"
msgstr[0] ""
@@ -8151,6 +8172,9 @@ msgstr ""
msgid "source"
msgstr ""
+msgid "source diff"
+msgstr ""
+
msgid "spendCommand|%{slash_command} will update the sum of the time spent."
msgstr ""
@@ -8172,6 +8196,9 @@ msgstr ""
msgid "view it on GitLab"
msgstr ""
+msgid "view the blob"
+msgstr ""
+
msgid "with %{additions} additions, %{deletions} deletions."
msgstr ""
diff --git a/spec/features/merge_request/user_creates_image_diff_notes_spec.rb b/spec/features/merge_request/user_creates_image_diff_notes_spec.rb
index d790bdc82ce..d19408ee87f 100644
--- a/spec/features/merge_request/user_creates_image_diff_notes_spec.rb
+++ b/spec/features/merge_request/user_creates_image_diff_notes_spec.rb
@@ -90,9 +90,6 @@ describe 'Merge request > User creates image diff notes', :js do
%w(inline parallel).each do |view|
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
Gitlab::Diff::Position.new(
old_path: path,
@@ -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) }
- describe 'creating a new diff note' do
+ shared_examples 'creates image diff note' do
before do
visit diffs_project_merge_request_path(project, merge_request, view: view)
+ wait_for_requests
+
create_image_diff_note
end
@@ -132,6 +131,32 @@ describe 'Merge request > User creates image diff notes', :js do
expect(page).to have_content('image diff test comment')
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
diff --git a/spec/features/merge_request/user_sees_diff_spec.rb b/spec/features/merge_request/user_sees_diff_spec.rb
index 0df9e4bbc1a..04b07525919 100644
--- a/spec/features/merge_request/user_sees_diff_spec.rb
+++ b/spec/features/merge_request/user_sees_diff_spec.rb
@@ -87,20 +87,6 @@ describe 'Merge request > User sees diff', :js do
let(:current_user) { project.owner }
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
file_content =
<<~CONTENT
@@ -136,5 +122,61 @@ describe 'Merge request > User sees diff', :js do
expect(page).to have_css(".line[lang='rust'] .k")
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
diff --git a/spec/features/projects/commits/user_browses_commits_spec.rb b/spec/features/projects/commits/user_browses_commits_spec.rb
index 2159adf49fc..574a8aefd63 100644
--- a/spec/features/projects/commits/user_browses_commits_spec.rb
+++ b/spec/features/projects/commits/user_browses_commits_spec.rb
@@ -93,7 +93,7 @@ describe 'User browses commits' 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(:raw_binary?).and_return(true)
+ allow_any_instance_of(Gitlab::Diff::File).to receive(:binary?).and_return(true)
visit(project_commit_path(project, commit))
diff --git a/spec/fixtures/api/schemas/entities/diff_viewer.json b/spec/fixtures/api/schemas/entities/diff_viewer.json
index 19780f49a88..81325cd86c6 100644
--- a/spec/fixtures/api/schemas/entities/diff_viewer.json
+++ b/spec/fixtures/api/schemas/entities/diff_viewer.json
@@ -1,8 +1,20 @@
{
"type": "object",
- "required": ["name"],
+ "required": [
+ "name"
+ ],
"properties": {
- "name": { "type": ["string"] }
+ "name": {
+ "type": [
+ "string"
+ ]
+ },
+ "error": {
+ "type": [
+ "string",
+ "null"
+ ]
+ }
},
"additionalProperties": false
}
diff --git a/spec/helpers/diff_helper_spec.rb b/spec/helpers/diff_helper_spec.rb
index 53c010fa0db..5396243f44d 100644
--- a/spec/helpers/diff_helper_spec.rb
+++ b/spec/helpers/diff_helper_spec.rb
@@ -256,43 +256,6 @@ describe DiffHelper do
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
it 'returns full path by default' do
expect(diff_file_path_text(diff_file)).to eq(diff_file.new_path)
diff --git a/spec/lib/gitlab/blob_helper_spec.rb b/spec/lib/gitlab/blob_helper_spec.rb
index 0b56f8687c3..e057385b35f 100644
--- a/spec/lib/gitlab/blob_helper_spec.rb
+++ b/spec/lib/gitlab/blob_helper_spec.rb
@@ -53,11 +53,11 @@ describe Gitlab::BlobHelper do
describe '#text?' do
it 'returns true' do
- expect(blob.text?).to be_truthy
+ expect(blob.text_in_repo?).to be_truthy
end
it 'returns false' do
- expect(large_blob.text?).to be_falsey
+ expect(large_blob.text_in_repo?).to be_falsey
end
end
diff --git a/spec/lib/gitlab/diff/file_spec.rb b/spec/lib/gitlab/diff/file_spec.rb
index b15d22c634a..862590268ca 100644
--- a/spec/lib/gitlab/diff/file_spec.rb
+++ b/spec/lib/gitlab/diff/file_spec.rb
@@ -310,7 +310,7 @@ describe Gitlab::Diff::File do
context 'when the content changed' do
context 'when the file represented by the diff file is binary' do
before do
- allow(diff_file).to receive(:raw_binary?).and_return(true)
+ allow(diff_file).to receive(:binary?).and_return(true)
end
it 'returns a No Preview viewer' do
@@ -345,7 +345,7 @@ describe Gitlab::Diff::File do
context 'when the file represented by the diff file is binary' do
before do
- allow(diff_file).to receive(:raw_binary?).and_return(true)
+ allow(diff_file).to receive(:binary?).and_return(true)
end
it 'returns an Added viewer' do
@@ -380,7 +380,7 @@ describe Gitlab::Diff::File do
context 'when the file represented by the diff file is binary' do
before do
- allow(diff_file).to receive(:raw_binary?).and_return(true)
+ allow(diff_file).to receive(:binary?).and_return(true)
end
it 'returns a Deleted viewer' 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(:renamed_file?).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
it 'returns a No Preview viewer' do
diff --git a/spec/lib/gitlab/diff/lines_unfolder_spec.rb b/spec/lib/gitlab/diff/lines_unfolder_spec.rb
index 8e00c8e0e30..f22c2c90334 100644
--- a/spec/lib/gitlab/diff/lines_unfolder_spec.rb
+++ b/spec/lib/gitlab/diff/lines_unfolder_spec.rb
@@ -185,7 +185,7 @@ describe Gitlab::Diff::LinesUnfolder do
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
Gitlab::Git::Diff.new(diff: raw_diff,
diff --git a/spec/lib/gitlab/git/blob_spec.rb b/spec/lib/gitlab/git/blob_spec.rb
index 80dd3dcc58e..1bcec04d28f 100644
--- a/spec/lib/gitlab/git/blob_spec.rb
+++ b/spec/lib/gitlab/git/blob_spec.rb
@@ -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.size).to eq(241) }
it { expect(blob.mode).to eq("100644") }
- it { expect(blob).not_to be_binary }
+ it { expect(blob).not_to be_binary_in_repo }
end
context 'file in root with leading slash' do
@@ -92,7 +92,7 @@ describe Gitlab::Git::Blob, :seed_helper do
end
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
@@ -123,7 +123,7 @@ describe Gitlab::Git::Blob, :seed_helper do
.with(hash_including(binary: true))
.and_call_original
- expect(blob).to be_binary
+ expect(blob).to be_binary_in_repo
end
end
end
@@ -196,7 +196,7 @@ describe Gitlab::Git::Blob, :seed_helper do
it { expect(blob.id).to eq('409f37c4f05865e4fb208c771485f211a22c4c2d') }
it { expect(blob.data).to eq('') }
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
diff --git a/spec/lib/gitlab/gitaly_client/blobs_stitcher_spec.rb b/spec/lib/gitlab/gitaly_client/blobs_stitcher_spec.rb
index 9db710e759e..742b2872c40 100644
--- a/spec/lib/gitlab/gitaly_client/blobs_stitcher_spec.rb
+++ b/spec/lib/gitlab/gitaly_client/blobs_stitcher_spec.rb
@@ -21,7 +21,7 @@ describe Gitlab::GitalyClient::BlobsStitcher do
expect(blobs[0].size).to eq(1642)
expect(blobs[0].commit_id).to eq('f00ba7')
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].mode).to eq('100644')
@@ -30,7 +30,7 @@ describe Gitlab::GitalyClient::BlobsStitcher do
expect(blobs[1].size).to eq(2461)
expect(blobs[1].commit_id).to eq('f00ba8')
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
diff --git a/spec/models/blob_spec.rb b/spec/models/blob_spec.rb
index e8c03b587e2..05cf242e84d 100644
--- a/spec/models/blob_spec.rb
+++ b/spec/models/blob_spec.rb
@@ -122,14 +122,14 @@ describe Blob do
end
end
- describe '#raw_binary?' do
+ describe '#binary?' do
context 'if the blob is stored externally' do
context 'if the extension has a rich viewer' do
context 'if the viewer is binary' do
it 'returns true' do
blob = fake_blob(path: 'file.pdf', lfs: true)
- expect(blob.raw_binary?).to be_truthy
+ expect(blob.binary?).to be_truthy
end
end
@@ -137,7 +137,7 @@ describe Blob do
it 'return false' do
blob = fake_blob(path: 'file.md', lfs: true)
- expect(blob.raw_binary?).to be_falsey
+ expect(blob.binary?).to be_falsey
end
end
end
@@ -148,7 +148,7 @@ describe Blob do
it 'returns false' do
blob = fake_blob(path: 'file.txt', lfs: true)
- expect(blob.raw_binary?).to be_falsey
+ expect(blob.binary?).to be_falsey
end
end
@@ -156,7 +156,7 @@ describe Blob do
it 'returns false' do
blob = fake_blob(path: 'file.ics', lfs: true)
- expect(blob.raw_binary?).to be_falsey
+ expect(blob.binary?).to be_falsey
end
end
end
@@ -166,7 +166,7 @@ describe Blob do
it 'returns false' do
blob = fake_blob(path: 'file.rb', lfs: true)
- expect(blob.raw_binary?).to be_falsey
+ expect(blob.binary?).to be_falsey
end
end
@@ -174,7 +174,7 @@ describe Blob do
it 'returns true' do
blob = fake_blob(path: 'file.exe', lfs: true)
- expect(blob.raw_binary?).to be_truthy
+ expect(blob.binary?).to be_truthy
end
end
end
@@ -184,7 +184,7 @@ describe Blob do
it 'returns false' do
blob = fake_blob(path: 'file.ini', lfs: true)
- expect(blob.raw_binary?).to be_falsey
+ expect(blob.binary?).to be_falsey
end
end
@@ -192,7 +192,7 @@ describe Blob do
it 'returns true' do
blob = fake_blob(path: 'file.wtf', lfs: true)
- expect(blob.raw_binary?).to be_truthy
+ expect(blob.binary?).to be_truthy
end
end
end
@@ -204,7 +204,7 @@ describe Blob do
it 'returns true' do
blob = fake_blob(path: 'file.pdf', binary: true)
- expect(blob.raw_binary?).to be_truthy
+ expect(blob.binary?).to be_truthy
end
end
@@ -212,7 +212,7 @@ describe Blob do
it 'return false' do
blob = fake_blob(path: 'file.md')
- expect(blob.raw_binary?).to be_falsey
+ expect(blob.binary?).to be_falsey
end
end
end
diff --git a/spec/models/diff_viewer/base_spec.rb b/spec/models/diff_viewer/base_spec.rb
index c90b32c5d77..f4efe5a7b3a 100644
--- a/spec/models/diff_viewer/base_spec.rb
+++ b/spec/models/diff_viewer/base_spec.rb
@@ -58,7 +58,7 @@ describe DiffViewer::Base do
context 'when the binaryness does not match' 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
it 'returns false' do
@@ -141,4 +141,25 @@ describe DiffViewer::Base do
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
diff --git a/spec/models/diff_viewer/server_side_spec.rb b/spec/models/diff_viewer/server_side_spec.rb
index 98a8f6d4cc9..86b14b6ebf3 100644
--- a/spec/models/diff_viewer/server_side_spec.rb
+++ b/spec/models/diff_viewer/server_side_spec.rb
@@ -32,4 +32,24 @@ describe DiffViewer::ServerSide do
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
diff --git a/spec/support/helpers/fake_blob_helpers.rb b/spec/support/helpers/fake_blob_helpers.rb
index bc9686ed9cf..801ca8b7412 100644
--- a/spec/support/helpers/fake_blob_helpers.rb
+++ b/spec/support/helpers/fake_blob_helpers.rb
@@ -23,7 +23,7 @@ module FakeBlobHelpers
0
end
- def binary?
+ def binary_in_repo?
@binary
end
diff --git a/spec/support/helpers/test_env.rb b/spec/support/helpers/test_env.rb
index d52c40ff4f1..d352a7cdf1a 100644
--- a/spec/support/helpers/test_env.rb
+++ b/spec/support/helpers/test_env.rb
@@ -62,7 +62,8 @@ module TestEnv
'between-create-delete-modify-move' => '3f5f443',
'after-create-delete-modify-move' => 'ba3faa7',
'with-codeowners' => '219560e',
- 'submodule_inside_folder' => 'b491b92'
+ 'submodule_inside_folder' => 'b491b92',
+ 'png-lfs' => 'fe42f41'
}.freeze
# gitlab-test-fork is a fork of gitlab-fork, but we don't necessarily