diff options
40 files changed, 1153 insertions, 39 deletions
diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index 2ae3a616933..06822747d11 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -124,6 +124,30 @@ 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 = namespace_project_blob_path(@project.namespace, @project, tree_join(diff_file.content_sha, diff_file.file_path)) + options << link_to('view the blob', blob_url) + + options + end + private def diff_btn(title, name, selected) diff --git a/app/models/blob_viewer/server_side.rb b/app/models/blob_viewer/server_side.rb index e6bcacf7f70..fbc1b520c01 100644 --- a/app/models/blob_viewer/server_side.rb +++ b/app/models/blob_viewer/server_side.rb @@ -13,14 +13,12 @@ module BlobViewer end def render_error - if blob.stored_externally? - # Files that are not stored in the repository, like LFS files and - # build artifacts, can only be rendered using a client-side viewer, - # since we do not want to read large amounts of data into memory on the - # server side. Client-side viewers use JS and can fetch the file from - # `blob_raw_url` using AJAX. - return :server_side_but_stored_externally - end + # Files that are not stored in the repository, like LFS files and + # build artifacts, can only be rendered using a client-side viewer, + # since we do not want to read large amounts of data into memory on the + # server side. Client-side viewers use JS and can fetch the file from + # `blob_raw_url` using AJAX. + return :server_side_but_stored_externally if blob.stored_externally? super end diff --git a/app/models/diff_viewer/added.rb b/app/models/diff_viewer/added.rb new file mode 100644 index 00000000000..1909e6ef9d8 --- /dev/null +++ b/app/models/diff_viewer/added.rb @@ -0,0 +1,8 @@ +module DiffViewer + class Added < Base + include Simple + include Static + + self.partial_name = 'added' + end +end diff --git a/app/models/diff_viewer/base.rb b/app/models/diff_viewer/base.rb new file mode 100644 index 00000000000..0cbe714288d --- /dev/null +++ b/app/models/diff_viewer/base.rb @@ -0,0 +1,87 @@ +module DiffViewer + class Base + PARTIAL_PATH_PREFIX = 'projects/diffs/viewers'.freeze + + class_attribute :partial_name, :type, :extensions, :file_types, :binary, :switcher_icon, :switcher_title + + # These limits relate to the sum of the old and new blob sizes. + # Limits related to the actual size of the diff are enforced in Gitlab::Diff::File. + class_attribute :collapse_limit, :size_limit + + delegate :partial_path, :loading_partial_path, :rich?, :simple?, :text?, :binary?, to: :class + + attr_reader :diff_file + + delegate :project, to: :diff_file + + def initialize(diff_file) + @diff_file = diff_file + @initially_binary = diff_file.binary? + end + + def self.partial_path + File.join(PARTIAL_PATH_PREFIX, partial_name) + end + + def self.rich? + type == :rich + end + + def self.simple? + type == :simple + end + + def self.binary? + binary + end + + def self.text? + !binary? + end + + def self.can_render?(diff_file, verify_binary: true) + can_render_blob?(diff_file.old_blob, verify_binary: verify_binary) && + can_render_blob?(diff_file.new_blob, verify_binary: verify_binary) + end + + def self.can_render_blob?(blob, verify_binary: true) + return true if blob.nil? + return false if verify_binary && binary? != blob.binary? + return true if extensions&.include?(blob.extension) + return true if file_types&.include?(blob.file_type) + + false + end + + def collapsed? + return @collapsed if defined?(@collapsed) + return @collapsed = true if diff_file.collapsed? + + @collapsed = !diff_file.expanded? && collapse_limit && diff_file.raw_size > collapse_limit + end + + def too_large? + return @too_large if defined?(@too_large) + return @too_large = true if diff_file.too_large? + + @too_large = size_limit && diff_file.raw_size > size_limit + end + + def binary_detected_after_load? + !@initially_binary && diff_file.binary? + 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. + def render_error + if too_large? + :too_large + end + end + + def prepare! + # To be overridden by subclasses + end + end +end diff --git a/app/models/diff_viewer/client_side.rb b/app/models/diff_viewer/client_side.rb new file mode 100644 index 00000000000..cf41d07f8eb --- /dev/null +++ b/app/models/diff_viewer/client_side.rb @@ -0,0 +1,10 @@ +module DiffViewer + module ClientSide + extend ActiveSupport::Concern + + included do + self.collapse_limit = 1.megabyte + self.size_limit = 10.megabytes + end + end +end diff --git a/app/models/diff_viewer/deleted.rb b/app/models/diff_viewer/deleted.rb new file mode 100644 index 00000000000..9c129bac694 --- /dev/null +++ b/app/models/diff_viewer/deleted.rb @@ -0,0 +1,8 @@ +module DiffViewer + class Deleted < Base + include Simple + include Static + + self.partial_name = 'deleted' + end +end diff --git a/app/models/diff_viewer/image.rb b/app/models/diff_viewer/image.rb new file mode 100644 index 00000000000..759d9a36ebb --- /dev/null +++ b/app/models/diff_viewer/image.rb @@ -0,0 +1,12 @@ +module DiffViewer + class Image < Base + include Rich + include ClientSide + + self.partial_name = 'image' + self.extensions = UploaderHelper::IMAGE_EXT + self.binary = true + self.switcher_icon = 'picture-o' + self.switcher_title = 'image diff' + end +end diff --git a/app/models/diff_viewer/mode_changed.rb b/app/models/diff_viewer/mode_changed.rb new file mode 100644 index 00000000000..d487d996f8d --- /dev/null +++ b/app/models/diff_viewer/mode_changed.rb @@ -0,0 +1,8 @@ +module DiffViewer + class ModeChanged < Base + include Simple + include Static + + self.partial_name = 'mode_changed' + end +end diff --git a/app/models/diff_viewer/no_preview.rb b/app/models/diff_viewer/no_preview.rb new file mode 100644 index 00000000000..5455fee4490 --- /dev/null +++ b/app/models/diff_viewer/no_preview.rb @@ -0,0 +1,9 @@ +module DiffViewer + class NoPreview < Base + include Simple + include Static + + self.partial_name = 'no_preview' + self.binary = true + end +end diff --git a/app/models/diff_viewer/not_diffable.rb b/app/models/diff_viewer/not_diffable.rb new file mode 100644 index 00000000000..4f9638626ea --- /dev/null +++ b/app/models/diff_viewer/not_diffable.rb @@ -0,0 +1,9 @@ +module DiffViewer + class NotDiffable < Base + include Simple + include Static + + self.partial_name = 'not_diffable' + self.binary = true + end +end diff --git a/app/models/diff_viewer/renamed.rb b/app/models/diff_viewer/renamed.rb new file mode 100644 index 00000000000..f1fbfd8c6d5 --- /dev/null +++ b/app/models/diff_viewer/renamed.rb @@ -0,0 +1,8 @@ +module DiffViewer + class Renamed < Base + include Simple + include Static + + self.partial_name = 'renamed' + end +end diff --git a/app/models/diff_viewer/rich.rb b/app/models/diff_viewer/rich.rb new file mode 100644 index 00000000000..3b0ca6e4cff --- /dev/null +++ b/app/models/diff_viewer/rich.rb @@ -0,0 +1,11 @@ +module DiffViewer + module Rich + extend ActiveSupport::Concern + + included do + self.type = :rich + self.switcher_icon = 'file-text-o' + 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 new file mode 100644 index 00000000000..aed1a0791b1 --- /dev/null +++ b/app/models/diff_viewer/server_side.rb @@ -0,0 +1,26 @@ +module DiffViewer + module ServerSide + extend ActiveSupport::Concern + + included do + self.collapse_limit = 1.megabyte + self.size_limit = 5.megabytes + end + + def prepare! + diff_file.old_blob&.load_all_data! + diff_file.new_blob&.load_all_data! + end + + def render_error + # Files that are not stored in the repository, like LFS files and + # build artifacts, can only be rendered using a client-side viewer, + # since we do not want to read large amounts of data into memory on the + # server side. Client-side viewers use JS and can fetch the file from + # `diff_file_blob_raw_path` and `diff_file_old_blob_raw_path` using AJAX. + return :server_side_but_stored_externally if diff_file.stored_externally? + + super + end + end +end diff --git a/app/models/diff_viewer/simple.rb b/app/models/diff_viewer/simple.rb new file mode 100644 index 00000000000..65750996ee4 --- /dev/null +++ b/app/models/diff_viewer/simple.rb @@ -0,0 +1,11 @@ +module DiffViewer + module Simple + extend ActiveSupport::Concern + + included do + self.type = :simple + self.switcher_icon = 'code' + self.switcher_title = 'source diff' + end + end +end diff --git a/app/models/diff_viewer/static.rb b/app/models/diff_viewer/static.rb new file mode 100644 index 00000000000..d761328b3f6 --- /dev/null +++ b/app/models/diff_viewer/static.rb @@ -0,0 +1,10 @@ +module DiffViewer + module Static + extend ActiveSupport::Concern + + # We can always render a static viewer, even if the diff is too large. + def render_error + nil + end + end +end diff --git a/app/models/diff_viewer/text.rb b/app/models/diff_viewer/text.rb new file mode 100644 index 00000000000..98f4b2aea2a --- /dev/null +++ b/app/models/diff_viewer/text.rb @@ -0,0 +1,15 @@ +module DiffViewer + class Text < Base + include Simple + include ServerSide + + self.partial_name = 'text' + self.binary = false + + # Since the text diff viewer doesn't render the old and new blobs in full, + # we only need the limits related to the actual size of the diff which are + # already enforced in Gitlab::Diff::File. + self.collapse_limit = nil + self.size_limit = nil + end +end diff --git a/app/views/projects/diffs/_collapsed.html.haml b/app/views/projects/diffs/_collapsed.html.haml new file mode 100644 index 00000000000..8772bd4705f --- /dev/null +++ b/app/views/projects/diffs/_collapsed.html.haml @@ -0,0 +1,5 @@ +- diff_file = viewer.diff_file +- url = url_for(params.merge(action: :diff_for_path, old_path: diff_file.old_path, new_path: diff_file.new_path, file_identifier: diff_file.file_identifier)) +.nothing-here-block.diff-collapsed{ data: { diff_for_path: url } } + This diff is collapsed. + %a.click-to-expand Click to expand it. diff --git a/app/views/projects/diffs/_content.html.haml b/app/views/projects/diffs/_content.html.haml index ec1c434a4b8..68f74f702ea 100644 --- a/app/views/projects/diffs/_content.html.haml +++ b/app/views/projects/diffs/_content.html.haml @@ -1,27 +1,2 @@ -- blob = diff_file.blob - .diff-content - - if diff_file.too_large? - .nothing-here-block This diff could not be displayed because it is too large. - - elsif blob.truncated? - .nothing-here-block The file could not be displayed because it is too large. - - elsif blob.readable_text? - - if !diff_file.diffable? - .nothing-here-block This diff was suppressed by a .gitattributes entry. - - elsif diff_file.collapsed? - - url = url_for(params.merge(action: :diff_for_path, old_path: diff_file.old_path, new_path: diff_file.new_path, file_identifier: diff_file.file_identifier)) - .nothing-here-block.diff-collapsed{ data: { diff_for_path: url } } - This diff is collapsed. - %a.click-to-expand - Click to expand it. - - elsif diff_file.diff_lines.length > 0 - = render "projects/diffs/viewers/text", diff_file: diff_file - - else - - if diff_file.mode_changed? - .nothing-here-block File mode changed - - elsif diff_file.renamed_file? - .nothing-here-block File moved - - elsif blob.image? - = render "projects/diffs/viewers/image", diff_file: diff_file - - else - .nothing-here-block No preview for this file type + = render 'projects/diffs/viewer', viewer: diff_file.rich_viewer || diff_file.simple_viewer diff --git a/app/views/projects/diffs/_render_error.html.haml b/app/views/projects/diffs/_render_error.html.haml new file mode 100644 index 00000000000..47a9ac3ee6b --- /dev/null +++ b/app/views/projects/diffs/_render_error.html.haml @@ -0,0 +1,6 @@ +.nothing-here-block + This #{viewer.switcher_title} could not be displayed because #{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. diff --git a/app/views/projects/diffs/_viewer.html.haml b/app/views/projects/diffs/_viewer.html.haml new file mode 100644 index 00000000000..5c4d1760871 --- /dev/null +++ b/app/views/projects/diffs/_viewer.html.haml @@ -0,0 +1,16 @@ +- hidden = local_assigns.fetch(:hidden, false) + +.diff-viewer{ data: { type: viewer.type }, class: ('hidden' if hidden) } + - if viewer.render_error + = render 'projects/diffs/render_error', viewer: viewer + - elsif viewer.collapsed? + = render 'projects/diffs/collapsed', viewer: viewer + - else + - viewer.prepare! + + -# In the rare case where the first kilobyte of the file looks like text, + -# but the file turns out to actually be binary after loading all data, + -# we fall back on the binary No Preview viewer. + - viewer = DiffViewer::NoPreview.new(viewer.diff_file) if viewer.binary_detected_after_load? + + = render viewer.partial_path, viewer: viewer diff --git a/app/views/projects/diffs/viewers/_added.html.haml b/app/views/projects/diffs/viewers/_added.html.haml new file mode 100644 index 00000000000..8004fe16688 --- /dev/null +++ b/app/views/projects/diffs/viewers/_added.html.haml @@ -0,0 +1,2 @@ +.nothing-here-block + File added diff --git a/app/views/projects/diffs/viewers/_deleted.html.haml b/app/views/projects/diffs/viewers/_deleted.html.haml new file mode 100644 index 00000000000..0ac7b4ca8f6 --- /dev/null +++ b/app/views/projects/diffs/viewers/_deleted.html.haml @@ -0,0 +1,2 @@ +.nothing-here-block + File deleted diff --git a/app/views/projects/diffs/viewers/_image.html.haml b/app/views/projects/diffs/viewers/_image.html.haml index ea75373581e..19d08181223 100644 --- a/app/views/projects/diffs/viewers/_image.html.haml +++ b/app/views/projects/diffs/viewers/_image.html.haml @@ -1,3 +1,4 @@ +- diff_file = viewer.diff_file - blob = diff_file.blob - old_blob = diff_file.old_blob - blob_raw_path = diff_file_blob_raw_path(diff_file) diff --git a/app/views/projects/diffs/viewers/_mode_changed.html.haml b/app/views/projects/diffs/viewers/_mode_changed.html.haml new file mode 100644 index 00000000000..69bc96bbdad --- /dev/null +++ b/app/views/projects/diffs/viewers/_mode_changed.html.haml @@ -0,0 +1,3 @@ +- diff_file = viewer.diff_file +.nothing-here-block + File mode changed from #{diff_file.a_mode} to #{diff_file.b_mode} diff --git a/app/views/projects/diffs/viewers/_no_preview.html.haml b/app/views/projects/diffs/viewers/_no_preview.html.haml new file mode 100644 index 00000000000..befe070af2b --- /dev/null +++ b/app/views/projects/diffs/viewers/_no_preview.html.haml @@ -0,0 +1,2 @@ +.nothing-here-block + No preview for this file type diff --git a/app/views/projects/diffs/viewers/_not_diffable.html.haml b/app/views/projects/diffs/viewers/_not_diffable.html.haml new file mode 100644 index 00000000000..b2c677ec59c --- /dev/null +++ b/app/views/projects/diffs/viewers/_not_diffable.html.haml @@ -0,0 +1,2 @@ +.nothing-here-block + This diff was suppressed by a .gitattributes entry. diff --git a/app/views/projects/diffs/viewers/_renamed.html.haml b/app/views/projects/diffs/viewers/_renamed.html.haml new file mode 100644 index 00000000000..ef05ee38d8d --- /dev/null +++ b/app/views/projects/diffs/viewers/_renamed.html.haml @@ -0,0 +1,2 @@ +.nothing-here-block + File moved diff --git a/app/views/projects/diffs/viewers/_text.html.haml b/app/views/projects/diffs/viewers/_text.html.haml index 120d3540223..509e68598c9 100644 --- a/app/views/projects/diffs/viewers/_text.html.haml +++ b/app/views/projects/diffs/viewers/_text.html.haml @@ -1,5 +1,5 @@ +- diff_file = viewer.diff_file - blob = diff_file.blob -- blob.load_all_data! - total_lines = blob.lines.size - total_lines -= 1 if total_lines > 0 && blob.lines.last.blank? - if diff_view == :parallel diff --git a/changelogs/unreleased/dm-diff-viewers.yml b/changelogs/unreleased/dm-diff-viewers.yml new file mode 100644 index 00000000000..e5b1352c8f1 --- /dev/null +++ b/changelogs/unreleased/dm-diff-viewers.yml @@ -0,0 +1,4 @@ +--- +title: Implement diff viewers +merge_request: +author: diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb index 4212a0dbe2e..d2863a4da71 100644 --- a/lib/gitlab/diff/file.rb +++ b/lib/gitlab/diff/file.rb @@ -5,7 +5,20 @@ module Gitlab delegate :new_file?, :deleted_file?, :renamed_file?, :old_path, :new_path, :a_mode, :b_mode, :mode_changed?, - :submodule?, :too_large?, :collapsed?, to: :diff, prefix: false + :submodule?, :expanded?, :too_large?, :collapsed?, :line_count, to: :diff, prefix: false + + # Finding a viewer for a diff file happens based only on extension and whether the + # diff file blobs are binary or text, which means 1 diff file should only be matched by 1 viewer, + # and the order of these viewers doesn't really matter. + # + # However, when the diff file blobs are LFS pointers, we cannot know for sure whether the + # file being pointed to is binary or text. In this case, we match only on + # extension, preferring binary viewers over text ones if both exist, since the + # large files referred to in "Large File Storage" are much more likely to be + # binary than text. + RICH_VIEWERS = [ + DiffViewer::Image + ].sort_by { |v| v.binary? ? 0 : 1 }.freeze def initialize(diff, repository:, diff_refs: nil, fallback_diff_refs: nil) @diff = diff @@ -177,6 +190,100 @@ module Gitlab def text? !binary? end + + def external_storage_error? + old_blob&.external_storage_error? || new_blob&.external_storage_error? + end + + def stored_externally? + old_blob&.stored_externally? || new_blob&.stored_externally? + end + + def external_storage + old_blob&.external_storage || new_blob&.external_storage + end + + def content_changed? + old_blob && new_blob && old_blob.id != new_blob.id + end + + def different_type? + old_blob && new_blob && old_blob.binary? != new_blob.binary? + end + + def size + [old_blob&.size, new_blob&.size].compact.sum + end + + def raw_size + [old_blob&.raw_size, new_blob&.raw_size].compact.sum + end + + def raw_binary? + old_blob&.raw_binary? || new_blob&.raw_binary? + end + + def raw_text? + !raw_binary? && !different_type? + end + + def simple_viewer + @simple_viewer ||= simple_viewer_class.new(self) + end + + def rich_viewer + return @rich_viewer if defined?(@rich_viewer) + + @rich_viewer = rich_viewer_class&.new(self) + end + + def rendered_as_text?(ignore_errors: true) + simple_viewer.is_a?(DiffViewer::Text) && (ignore_errors || simple_viewer.render_error.nil?) + end + + private + + def simple_viewer_class + return DiffViewer::NotDiffable unless diffable? + + if content_changed? + if raw_text? + DiffViewer::Text + else + DiffViewer::NoPreview + end + elsif new_file? + if raw_text? + DiffViewer::Text + else + DiffViewer::Added + end + elsif deleted_file? + if raw_text? + DiffViewer::Text + else + DiffViewer::Deleted + end + elsif renamed_file? + DiffViewer::Renamed + elsif mode_changed? + DiffViewer::ModeChanged + end + end + + def rich_viewer_class + viewer_class_from(RICH_VIEWERS) + end + + def viewer_class_from(classes) + return unless diffable? + return if different_type? || external_storage_error? + return unless new_file? || deleted_file? || content_changed? + + verify_binary = !stored_externally? + + classes.find { |viewer_class| viewer_class.can_render?(self, verify_binary: verify_binary) } + end end end end diff --git a/lib/gitlab/git/diff.rb b/lib/gitlab/git/diff.rb index 88ad760bea3..4b689f0e94f 100644 --- a/lib/gitlab/git/diff.rb +++ b/lib/gitlab/git/diff.rb @@ -17,6 +17,8 @@ module Gitlab attr_accessor :expanded + alias_method :expanded?, :expanded + # We need this accessor because of `to_hash` and `init_from_hash` attr_accessor :too_large diff --git a/lib/gitlab/git/diff_collection.rb b/lib/gitlab/git/diff_collection.rb index 334e06a6eca..555894907cc 100644 --- a/lib/gitlab/git/diff_collection.rb +++ b/lib/gitlab/git/diff_collection.rb @@ -97,7 +97,7 @@ module Gitlab diff = Gitlab::Git::Diff.new(raw, expanded: expanded) - if !expanded && over_safe_limits?(i) + if !expanded && over_safe_limits?(i) && diff.line_count > 0 diff.collapse! end diff --git a/spec/features/expand_collapse_diffs_spec.rb b/spec/features/expand_collapse_diffs_spec.rb index c4d5077e5e1..07521f50968 100644 --- a/spec/features/expand_collapse_diffs_spec.rb +++ b/spec/features/expand_collapse_diffs_spec.rb @@ -262,7 +262,7 @@ feature 'Expand and collapse diffs', js: true, feature: true do # Wait for elements to appear to ensure full page reload expect(page).to have_content('This diff was suppressed by a .gitattributes entry') - expect(page).to have_content('This diff could not be displayed because it is too large.') + expect(page).to have_content('This source diff could not be displayed because it is too large.') expect(page).to have_content('too_large_image.jpg') find('.note-textarea') diff --git a/spec/features/projects/blobs/blob_show_spec.rb b/spec/features/projects/blobs/blob_show_spec.rb index 45fdb36e506..71ffa352f80 100644 --- a/spec/features/projects/blobs/blob_show_spec.rb +++ b/spec/features/projects/blobs/blob_show_spec.rb @@ -17,6 +17,7 @@ feature 'File blob', :js, feature: true do it 'displays the blob' do aggregate_failures do # shows highlighted Ruby code + expect(page).to have_css(".js-syntax-highlight") expect(page).to have_content("require 'fileutils'") # does not show a viewer switcher @@ -71,6 +72,7 @@ feature 'File blob', :js, feature: true do expect(page).to have_selector('.blob-viewer[data-type="rich"]', visible: false) # shows highlighted Markdown code + expect(page).to have_css(".js-syntax-highlight") expect(page).to have_content("[PEP-8](http://www.python.org/dev/peps/pep-0008/)") # shows an enabled copy button @@ -114,6 +116,7 @@ feature 'File blob', :js, feature: true do expect(page).to have_selector('#LC1.hll') # shows highlighted Markdown code + expect(page).to have_css(".js-syntax-highlight") expect(page).to have_content("[PEP-8](http://www.python.org/dev/peps/pep-0008/)") # shows an enabled copy button diff --git a/spec/features/projects/diffs/diff_show_spec.rb b/spec/features/projects/diffs/diff_show_spec.rb new file mode 100644 index 00000000000..48b7f1e0f34 --- /dev/null +++ b/spec/features/projects/diffs/diff_show_spec.rb @@ -0,0 +1,133 @@ +require 'spec_helper' + +feature 'Diff file viewer', :js, feature: true do + let(:project) { create(:project, :public, :repository) } + + def visit_commit(sha, anchor: nil) + visit namespace_project_commit_path(project.namespace, project, sha, anchor: anchor) + + wait_for_requests + end + + context 'Ruby file' do + before do + visit_commit('570e7b2abdd848b95f2f578043fc23bd6f6fd24d') + end + + it 'shows highlighted Ruby code' do + within('.diff-file[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd"]') do + expect(page).to have_css(".js-syntax-highlight") + expect(page).to have_content("def popen(cmd, path=nil)") + end + end + end + + context 'Ruby file (stored in LFS)' do + before do + project.add_master(project.creator) + + @commit_id = Files::CreateService.new( + project, + project.creator, + start_branch: 'master', + branch_name: 'master', + commit_message: "Add Ruby file in LFS", + file_path: 'files/lfs/ruby.rb', + file_content: project.repository.blob_at('master', 'files/lfs/lfs_object.iso').data + ).execute[:result] + end + + 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) + + visit_commit(@commit_id) + end + + it 'shows an error message' do + expect(page).to have_content('This source diff could not be displayed because it is stored in LFS. You can view the blob instead.') + end + end + + context 'when LFS is disabled on the project' do + before do + visit_commit(@commit_id) + end + + it 'displays the diff' do + expect(page).to have_content('size 1575078') + end + end + end + + context 'Image file' do + before do + visit_commit('2f63565e7aac07bcdadb654e253078b727143ec4') + end + + it 'shows a rendered image' do + within('.diff-file[id="e986451b8f7397b617dbb6fffcb5539328c56921"]') do + expect(page).to have_css('img[alt="files/images/6049019_460s.jpg"]') + end + end + end + + context 'ISO file (stored in LFS)' do + 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) + + visit_commit('048721d90c449b244b7b4c53a9186b04330174ec') + end + + it 'shows that file was added' do + expect(page).to have_content('File added') + end + end + + context 'when LFS is disabled on the project' do + before do + visit_commit('048721d90c449b244b7b4c53a9186b04330174ec') + end + + it 'displays the diff' do + expect(page).to have_content('size 1575078') + end + end + end + + context 'ZIP file' do + before do + visit_commit('ae73cb07c9eeaf35924a10f713b364d32b2dd34f') + end + + it 'shows that file was added' do + expect(page).to have_content('File added') + end + end + + context 'binary file that appears to be text in the first 1024 bytes' do + before do + visit_commit('7b1cf4336b528e0f3d1d140ee50cafdbc703597c') + end + + it 'shows the diff is collapsed' do + expect(page).to have_content('This diff is collapsed. Click to expand it.') + end + + context 'expanding the diff' do + before do + # We can't use `click_link` because the "link" doesn't have an `href`. + find('a.click-to-expand').click + + wait_for_requests + end + + it 'shows there is no preview' do + expect(page).to have_content('No preview for this file type') + end + end + end +end diff --git a/spec/helpers/diff_helper_spec.rb b/spec/helpers/diff_helper_spec.rb index a74615e07f9..0ac030d3171 100644 --- a/spec/helpers/diff_helper_spec.rb +++ b/spec/helpers/diff_helper_spec.rb @@ -8,7 +8,7 @@ describe DiffHelper do let(:commit) { project.commit(sample_commit.id) } let(:diffs) { commit.raw_diffs } let(:diff) { diffs.first } - let(:diff_refs) { [commit.parent, commit] } + let(:diff_refs) { commit.diff_refs } let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: diff_refs, repository: repository) } describe 'diff_view' do @@ -207,4 +207,41 @@ describe DiffHelper do expect(output).not_to have_css 'td:nth-child(3)' 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 end diff --git a/spec/lib/gitlab/diff/file_spec.rb b/spec/lib/gitlab/diff/file_spec.rb index a9953bb0d01..f289131cc3a 100644 --- a/spec/lib/gitlab/diff/file_spec.rb +++ b/spec/lib/gitlab/diff/file_spec.rb @@ -92,4 +92,305 @@ describe Gitlab::Diff::File, lib: true do expect(diff_file.diffable?).to be_falsey end end + + describe '#content_changed?' do + context 'when created' do + let(:commit) { project.commit('33f3729a45c02fc67d00adb1b8bca394b0e761d9') } + let(:diff_file) { commit.diffs.diff_file_with_new_path('files/images/6049019_460s.jpg') } + + it 'returns false' do + expect(diff_file.content_changed?).to be_falsey + end + end + + context 'when deleted' do + let(:commit) { project.commit('d59c60028b053793cecfb4022de34602e1a9218e') } + let(:diff_file) { commit.diffs.diff_file_with_old_path('files/js/commit.js.coffee') } + + it 'returns false' do + expect(diff_file.content_changed?).to be_falsey + end + end + + context 'when renamed' do + let(:commit) { project.commit('6907208d755b60ebeacb2e9dfea74c92c3449a1f') } + let(:diff_file) { commit.diffs.diff_file_with_new_path('files/js/commit.coffee') } + + before do + allow(diff_file.new_blob).to receive(:id).and_return(diff_file.old_blob.id) + end + + it 'returns false' do + expect(diff_file.content_changed?).to be_falsey + end + end + + context 'when content changed' do + context 'when binary' do + let(:commit) { project.commit('2f63565e7aac07bcdadb654e253078b727143ec4') } + let(:diff_file) { commit.diffs.diff_file_with_new_path('files/images/6049019_460s.jpg') } + + it 'returns true' do + expect(diff_file.content_changed?).to be_truthy + end + end + + context 'when not binary' do + let(:commit) { project.commit('570e7b2abdd848b95f2f578043fc23bd6f6fd24d') } + let(:diff_file) { commit.diffs.diff_file_with_new_path('files/ruby/popen.rb') } + + it 'returns true' do + expect(diff_file.content_changed?).to be_truthy + end + end + end + end + + describe '#simple_viewer' do + context 'when the file is not diffable' do + before do + allow(diff_file).to receive(:diffable?).and_return(false) + end + + it 'returns a Not Diffable viewer' do + expect(diff_file.simple_viewer).to be_a(DiffViewer::NotDiffable) + end + end + + 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) + end + + it 'returns a No Preview viewer' do + expect(diff_file.simple_viewer).to be_a(DiffViewer::NoPreview) + end + end + + context 'when the diff file old and new blob types are different' do + before do + allow(diff_file).to receive(:different_type?).and_return(true) + end + + it 'returns a No Preview viewer' do + expect(diff_file.simple_viewer).to be_a(DiffViewer::NoPreview) + end + end + + context 'when the file represented by the diff file is text-based' do + it 'returns a text viewer' do + expect(diff_file.simple_viewer).to be_a(DiffViewer::Text) + end + end + end + + context 'when created' do + let(:commit) { project.commit('913c66a37b4a45b9769037c55c2d238bd0942d2e') } + let(:diff_file) { commit.diffs.diff_file_with_new_path('files/ruby/popen.rb') } + + before do + allow(diff_file).to receive(:content_changed?).and_return(nil) + end + + context 'when the file represented by the diff file is binary' do + before do + allow(diff_file).to receive(:raw_binary?).and_return(true) + end + + it 'returns an Added viewer' do + expect(diff_file.simple_viewer).to be_a(DiffViewer::Added) + end + end + + context 'when the diff file old and new blob types are different' do + before do + allow(diff_file).to receive(:different_type?).and_return(true) + end + + it 'returns an Added viewer' do + expect(diff_file.simple_viewer).to be_a(DiffViewer::Added) + end + end + + context 'when the file represented by the diff file is text-based' do + it 'returns a text viewer' do + expect(diff_file.simple_viewer).to be_a(DiffViewer::Text) + end + end + end + + context 'when deleted' do + let(:commit) { project.commit('d59c60028b053793cecfb4022de34602e1a9218e') } + let(:diff_file) { commit.diffs.diff_file_with_old_path('files/js/commit.js.coffee') } + + before do + allow(diff_file).to receive(:content_changed?).and_return(nil) + end + + context 'when the file represented by the diff file is binary' do + before do + allow(diff_file).to receive(:raw_binary?).and_return(true) + end + + it 'returns a Deleted viewer' do + expect(diff_file.simple_viewer).to be_a(DiffViewer::Deleted) + end + end + + context 'when the diff file old and new blob types are different' do + before do + allow(diff_file).to receive(:different_type?).and_return(true) + end + + it 'returns a Deleted viewer' do + expect(diff_file.simple_viewer).to be_a(DiffViewer::Deleted) + end + end + + context 'when the file represented by the diff file is text-based' do + it 'returns a text viewer' do + expect(diff_file.simple_viewer).to be_a(DiffViewer::Text) + end + end + end + + context 'when renamed' do + let(:commit) { project.commit('6907208d755b60ebeacb2e9dfea74c92c3449a1f') } + let(:diff_file) { commit.diffs.diff_file_with_new_path('files/js/commit.coffee') } + + before do + allow(diff_file).to receive(:content_changed?).and_return(nil) + end + + it 'returns a Renamed viewer' do + expect(diff_file.simple_viewer).to be_a(DiffViewer::Renamed) + end + end + + context 'when mode changed' do + before do + allow(diff_file).to receive(:content_changed?).and_return(nil) + allow(diff_file).to receive(:mode_changed?).and_return(true) + end + + it 'returns a Mode Changed viewer' do + expect(diff_file.simple_viewer).to be_a(DiffViewer::ModeChanged) + end + end + end + + describe '#rich_viewer' do + let(:commit) { project.commit('2f63565e7aac07bcdadb654e253078b727143ec4') } + let(:diff_file) { commit.diffs.diff_file_with_new_path('files/images/6049019_460s.jpg') } + + context 'when the diff file has a matching viewer' do + context 'when the diff file content did not change' do + before do + allow(diff_file).to receive(:content_changed?).and_return(false) + end + + it 'returns nil' do + expect(diff_file.rich_viewer).to be_nil + end + end + + context 'when the diff file is not diffable' do + before do + allow(diff_file).to receive(:diffable?).and_return(false) + end + + it 'returns nil' do + expect(diff_file.rich_viewer).to be_nil + end + end + + context 'when the diff file old and new blob types are different' do + before do + allow(diff_file).to receive(:different_type?).and_return(true) + end + + it 'returns nil' do + expect(diff_file.rich_viewer).to be_nil + end + end + + context 'when the diff file has an external storage error' do + before do + allow(diff_file).to receive(:external_storage_error?).and_return(true) + end + + it 'returns nil' do + expect(diff_file.rich_viewer).to be_nil + end + end + + context 'when everything is right' do + it 'returns the viewer' do + expect(diff_file.rich_viewer).to be_a(DiffViewer::Image) + end + end + end + + context 'when the diff file does not have a matching viewer' do + let(:commit) { project.commit('570e7b2abdd848b95f2f578043fc23bd6f6fd24d') } + let(:diff_file) { commit.diffs.diff_file_with_new_path('files/ruby/popen.rb') } + + it 'returns nil' do + expect(diff_file.rich_viewer).to be_nil + end + end + end + + describe '#rendered_as_text?' do + context 'when the simple viewer is text-based' do + let(:commit) { project.commit('570e7b2abdd848b95f2f578043fc23bd6f6fd24d') } + let(:diff_file) { commit.diffs.diff_file_with_new_path('files/ruby/popen.rb') } + + context 'when ignoring errors' do + context 'when the viewer has render errors' do + before do + diff_file.diff.too_large! + end + + it 'returns true' do + expect(diff_file.rendered_as_text?).to be_truthy + end + end + + context "when the viewer doesn't have render errors" do + it 'returns true' do + expect(diff_file.rendered_as_text?).to be_truthy + end + end + end + + context 'when not ignoring errors' do + context 'when the viewer has render errors' do + before do + diff_file.diff.too_large! + end + + it 'returns false' do + expect(diff_file.rendered_as_text?(ignore_errors: false)).to be_falsey + end + end + + context "when the viewer doesn't have render errors" do + it 'returns true' do + expect(diff_file.rendered_as_text?(ignore_errors: false)).to be_truthy + end + end + end + end + + context 'when the simple viewer is binary' do + let(:commit) { project.commit('2f63565e7aac07bcdadb654e253078b727143ec4') } + let(:diff_file) { commit.diffs.diff_file_with_new_path('files/images/6049019_460s.jpg') } + + it 'returns false' do + expect(diff_file.rendered_as_text?).to be_falsey + end + end + end end diff --git a/spec/models/diff_viewer/base_spec.rb b/spec/models/diff_viewer/base_spec.rb new file mode 100644 index 00000000000..3755f4a56f3 --- /dev/null +++ b/spec/models/diff_viewer/base_spec.rb @@ -0,0 +1,150 @@ +require 'spec_helper' + +describe DiffViewer::Base, model: true do + include FakeBlobHelpers + + let(:project) { create(:project, :repository) } + let(:commit) { project.commit('570e7b2abdd848b95f2f578043fc23bd6f6fd24d') } + let(:diff_file) { commit.diffs.diff_file_with_new_path('files/ruby/popen.rb') } + + let(:viewer_class) do + Class.new(described_class) do + include DiffViewer::ServerSide + + self.extensions = %w(jpg) + self.binary = true + self.collapse_limit = 1.megabyte + self.size_limit = 5.megabytes + end + end + + let(:viewer) { viewer_class.new(diff_file) } + + describe '.can_render?' do + context 'when the extension is supported' do + let(:commit) { project.commit('2f63565e7aac07bcdadb654e253078b727143ec4') } + let(:diff_file) { commit.diffs.diff_file_with_new_path('files/images/6049019_460s.jpg') } + + context 'when the binaryness matches' do + it 'returns true' do + expect(viewer_class.can_render?(diff_file)).to be_truthy + end + end + + context 'when the binaryness does not match' do + before do + allow(diff_file.old_blob).to receive(:binary?).and_return(false) + allow(diff_file.new_blob).to receive(:binary?).and_return(false) + end + + it 'returns false' do + expect(viewer_class.can_render?(diff_file)).to be_falsey + end + end + end + + context 'when the file type is supported' do + let(:commit) { project.commit('1a0b36b3cdad1d2ee32457c102a8c0b7056fa863') } + let(:diff_file) { commit.diffs.diff_file_with_new_path('LICENSE') } + + before do + viewer_class.file_types = %i(license) + viewer_class.binary = false + end + + context 'when the binaryness matches' do + it 'returns true' do + expect(viewer_class.can_render?(diff_file)).to be_truthy + end + end + + context 'when the binaryness does not match' do + before do + allow(diff_file.old_blob).to receive(:binary?).and_return(true) + allow(diff_file.new_blob).to receive(:binary?).and_return(true) + end + + it 'returns false' do + expect(viewer_class.can_render?(diff_file)).to be_falsey + end + end + end + + context 'when the extension and file type are not supported' do + it 'returns false' do + expect(viewer_class.can_render?(diff_file)).to be_falsey + end + end + + context 'when the file was renamed and only the old blob is supported' do + let(:commit) { project.commit('2f63565e7aac07bcdadb654e253078b727143ec4') } + let(:diff_file) { commit.diffs.diff_file_with_new_path('files/images/6049019_460s.jpg') } + + before do + allow(diff_file).to receive(:renamed_file?).and_return(true) + allow(diff_file.new_blob).to receive(:extension).and_return('jpeg') + end + + it 'returns false' do + expect(viewer_class.can_render?(diff_file)).to be_falsey + end + end + end + + describe '#collapsed?' do + context 'when the combined blob size is larger than the collapse limit' do + before do + allow(diff_file.old_blob).to receive(:raw_size).and_return(512.kilobytes) + allow(diff_file.new_blob).to receive(:raw_size).and_return(513.kilobytes) + end + + it 'returns true' do + expect(viewer.collapsed?).to be_truthy + end + end + + context 'when the combined blob size is smaller than the collapse limit' do + it 'returns false' do + expect(viewer.collapsed?).to be_falsey + end + end + end + + describe '#too_large?' do + context 'when the combined blob size is larger than the size limit' do + before do + allow(diff_file.old_blob).to receive(:raw_size).and_return(2.megabytes) + allow(diff_file.new_blob).to receive(:raw_size).and_return(4.megabytes) + end + + it 'returns true' do + expect(viewer.too_large?).to be_truthy + end + end + + context 'when the blob size is smaller than the size limit' do + it 'returns false' do + expect(viewer.too_large?).to be_falsey + end + end + end + + describe '#render_error' do + context 'when the combined blob size is larger than the size limit' do + before do + allow(diff_file.old_blob).to receive(:raw_size).and_return(2.megabytes) + allow(diff_file.new_blob).to receive(:raw_size).and_return(4.megabytes) + end + + it 'returns :too_large' do + expect(viewer.render_error).to eq(:too_large) + end + end + + context 'when the combined blob size is smaller than the size limit' do + it 'returns nil' do + expect(viewer.render_error).to be_nil + end + end + end +end diff --git a/spec/models/diff_viewer/server_side_spec.rb b/spec/models/diff_viewer/server_side_spec.rb new file mode 100644 index 00000000000..2d926e06936 --- /dev/null +++ b/spec/models/diff_viewer/server_side_spec.rb @@ -0,0 +1,36 @@ +require 'spec_helper' + +describe DiffViewer::ServerSide, model: true do + let(:project) { create(:project, :repository) } + let(:commit) { project.commit('570e7b2abdd848b95f2f578043fc23bd6f6fd24d') } + let(:diff_file) { commit.diffs.diff_file_with_new_path('files/ruby/popen.rb') } + + let(:viewer_class) do + Class.new(DiffViewer::Base) do + include DiffViewer::ServerSide + end + end + + subject { viewer_class.new(diff_file) } + + describe '#prepare!' do + it 'loads all diff file data' do + expect(diff_file.old_blob).to receive(:load_all_data!) + expect(diff_file.new_blob).to receive(:load_all_data!) + + subject.prepare! + end + end + + describe '#render_error' do + context 'when the diff file is stored externally' do + before do + allow(diff_file).to receive(:stored_externally?).and_return(true) + end + + it 'return :server_side_but_stored_externally' do + expect(subject.render_error).to eq(:server_side_but_stored_externally) + end + end + end +end diff --git a/spec/views/projects/diffs/_viewer.html.haml_spec.rb b/spec/views/projects/diffs/_viewer.html.haml_spec.rb new file mode 100644 index 00000000000..32469202508 --- /dev/null +++ b/spec/views/projects/diffs/_viewer.html.haml_spec.rb @@ -0,0 +1,71 @@ +require 'spec_helper' + +describe 'projects/diffs/_viewer.html.haml', :view do + include FakeBlobHelpers + + let(:project) { create(:project, :repository) } + let(:commit) { project.commit('570e7b2abdd848b95f2f578043fc23bd6f6fd24d') } + let(:diff_file) { commit.diffs.diff_file_with_new_path('files/ruby/popen.rb') } + + let(:viewer_class) do + Class.new(DiffViewer::Base) do + include DiffViewer::Rich + + self.partial_name = 'text' + end + end + + let(:viewer) { viewer_class.new(diff_file) } + + before do + assign(:project, project) + + controller.params[:controller] = 'projects/commit' + controller.params[:action] = 'show' + controller.params[:namespace_id] = project.namespace.to_param + controller.params[:project_id] = project.to_param + controller.params[:id] = commit.id + end + + def render_view + render partial: 'projects/diffs/viewer', locals: { viewer: viewer } + end + + context 'when there is a render error' do + before do + allow(viewer).to receive(:render_error).and_return(:too_large) + end + + it 'renders the error' do + render_view + + expect(view).to render_template('projects/diffs/_render_error') + end + end + + context 'when the viewer is collapsed' do + before do + allow(diff_file).to receive(:collapsed?).and_return(true) + end + + it 'renders the collapsed view' do + render_view + + expect(view).to render_template('projects/diffs/_collapsed') + end + end + + context 'when there is no render error' do + it 'prepares the viewer' do + expect(viewer).to receive(:prepare!) + + render_view + end + + it 'renders the viewer' do + render_view + + expect(view).to render_template('projects/diffs/viewers/_text') + end + end +end |