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.

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>

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?,

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' }

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"

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)

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

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

app/models/concerns/blob_like.rb

@@ -28,7 +28,7 @@ module BlobLike
     nil
   end
-  def binary?
+  def binary_in_repo?
     false
   end

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

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

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

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

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

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

app/views/projects/diffs/_render_error.html.haml

 .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

changelogs/unreleased/fj-fix-lfs-image-comments-diffs.yml

+---
+title: Fix bug commenting on LFS images
+merge_request: 23812
+author:
+type: fixed

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

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

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

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 ""