diff options
author | Francisco Javier López <fjlopez@gitlab.com> | 2018-12-13 18:49:05 +0100 |
---|---|---|
committer | Francisco Javier López <fjlopez@gitlab.com> | 2018-12-27 16:51:07 +0100 |
commit | 5a3e6fdff96f50cb293a8c9fe64ccbf59619936f (patch) | |
tree | 2a5d109ec7bfc07336ad7a155f3234ba4e0f6580 | |
parent | 77909a88460bbc864a5f95f3fa66053eb6cab5a8 (diff) | |
download | gitlab-ce-5a3e6fdff96f50cb293a8c9fe64ccbf59619936f.tar.gz |
Fixing image lfs bug and also displaying text lfs
This commit, introduced in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/23812,
fixes a problem creating a displaying image diff notes when the image
is stored in LFS. The main problem was that `Gitlab::Diff::File` was
returning an invalid valid in `text?` for this kind of files.
It also fixes a rendering problem with other LFS files, like text
ones. They LFS pointer shouldn't be shown when LFS is enabled
for the project, but they were.
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 |