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.

15 files changed, 72 insertions, 50 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