diff options
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 |