From bc14e4ed1024efa1e0a411bd59e1339fb1af20c0 Mon Sep 17 00:00:00 2001 From: Mark Chao Date: Tue, 11 Sep 2018 12:37:44 +0800 Subject: [PATCH] Move :plain option to Highlight class This is to DRY the repeated file size check. Move spec and constants to Highlight --- app/helpers/blob_helper.rb | 1 - app/models/blob.rb | 6 -- app/presenters/blob_presenter.rb | 2 +- .../search/results/_snippet_blob.html.haml | 2 +- lib/gitlab/highlight.rb | 3 + lib/gitlab/search_results.rb | 4 -- spec/helpers/blob_helper_spec.rb | 58 ++---------------- spec/lib/gitlab/highlight_spec.rb | 60 +++++++++++++++++++ spec/presenters/blob_presenter_spec.rb | 26 ++------ 9 files changed, 74 insertions(+), 88 deletions(-) diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb index 52cf9d12287..638744a1426 100644 --- a/app/helpers/blob_helper.rb +++ b/app/helpers/blob_helper.rb @@ -2,7 +2,6 @@ module BlobHelper def highlight(file_name, file_content, language: nil, plain: false) - plain ||= file_content.length > Blob::MAXIMUM_TEXT_HIGHLIGHT_SIZE highlighted = Gitlab::Highlight.highlight(file_name, file_content, plain: plain, language: language) raw %(
#{highlighted}
) diff --git a/app/models/blob.rb b/app/models/blob.rb index 425ba69c073..57c813e3775 100644 --- a/app/models/blob.rb +++ b/app/models/blob.rb @@ -7,8 +7,6 @@ class Blob < SimpleDelegator CACHE_TIME = 60 # Cache raw blobs referred to by a (mutable) ref for 1 minute CACHE_TIME_IMMUTABLE = 3600 # Cache blobs referred to by an immutable reference for 1 hour - MAXIMUM_TEXT_HIGHLIGHT_SIZE = 1.megabyte - # Finding a viewer for a blob happens based only on extension and whether the # blob is binary or text, which means 1 blob should only be matched by 1 viewer, # and the order of these viewers doesn't really matter. @@ -123,10 +121,6 @@ class Blob < SimpleDelegator end end - def no_highlighting? - raw_size && raw_size > MAXIMUM_TEXT_HIGHLIGHT_SIZE - end - def empty? raw_size == 0 end diff --git a/app/presenters/blob_presenter.rb b/app/presenters/blob_presenter.rb index 2bca413876c..9980f6cd8a6 100644 --- a/app/presenters/blob_presenter.rb +++ b/app/presenters/blob_presenter.rb @@ -8,7 +8,7 @@ class BlobPresenter < Gitlab::View::Presenter::Simple blob.path, blob.data, language: blob.language_from_gitattributes, - plain: (plain || blob.no_highlighting?) + plain: plain ) end end diff --git a/app/views/search/results/_snippet_blob.html.haml b/app/views/search/results/_snippet_blob.html.haml index 92ea6f281f4..b4ecd7bbae9 100644 --- a/app/views/search/results/_snippet_blob.html.haml +++ b/app/views/search/results/_snippet_blob.html.haml @@ -39,7 +39,7 @@ .blob-content - snippet_chunks.each do |chunk| - unless chunk[:data].empty? - = highlight(snippet.file_name, chunk[:data], plain: snippet.blob.no_highlighting?) + = highlight(snippet.file_name, chunk[:data]) - else .file-content.code .nothing-here-block Empty file diff --git a/lib/gitlab/highlight.rb b/lib/gitlab/highlight.rb index d28223194cc..a4e60bbd828 100644 --- a/lib/gitlab/highlight.rb +++ b/lib/gitlab/highlight.rb @@ -4,6 +4,7 @@ module Gitlab class Highlight TIMEOUT_BACKGROUND = 30.seconds TIMEOUT_FOREGROUND = 3.seconds + MAXIMUM_TEXT_HIGHLIGHT_SIZE = 1.megabyte def self.highlight(blob_name, blob_content, language: nil, plain: false) new(blob_name, blob_content, language: language) @@ -20,6 +21,8 @@ module Gitlab end def highlight(text, continue: true, plain: false) + plain ||= text.length > MAXIMUM_TEXT_HIGHLIGHT_SIZE + highlighted_text = highlight_text(text, continue: continue, plain: plain) highlighted_text = link_dependencies(text, highlighted_text) if blob_name highlighted_text diff --git a/lib/gitlab/search_results.rb b/lib/gitlab/search_results.rb index 3dbb0608a4f..6c86ad11385 100644 --- a/lib/gitlab/search_results.rb +++ b/lib/gitlab/search_results.rb @@ -23,10 +23,6 @@ module Gitlab filename end - def no_highlighting? - false - end - # Since search results often contain many items, # not triggering lookup can avoid n+1 queries. def language_from_gitattributes diff --git a/spec/helpers/blob_helper_spec.rb b/spec/helpers/blob_helper_spec.rb index 1c216b3fe97..f709f152c92 100644 --- a/spec/helpers/blob_helper_spec.rb +++ b/spec/helpers/blob_helper_spec.rb @@ -3,63 +3,13 @@ require 'spec_helper' describe BlobHelper do include TreeHelper - let(:blob_name) { 'test.lisp' } - let(:no_context_content) { ":type \"assem\"))" } - let(:blob_content) { "(make-pathname :defaults name\n#{no_context_content}" } - let(:split_content) { blob_content.split("\n") } - let(:multiline_content) do - %q( - def test(input): - """This is line 1 of a multi-line comment. - This is line 2. - """ - ) - end - describe '#highlight' do - it 'returns plaintext for unknown lexer context' do - result = helper.highlight(blob_name, no_context_content) - expect(result).to eq(%[
:type "assem"))
]) + it 'wraps highlighted content' do + expect(helper.highlight('test.rb', '52')).to eq(%q[
52
]) end - it 'returns plaintext for long blobs' do - stub_const('Blob::MAXIMUM_TEXT_HIGHLIGHT_SIZE', 1) - result = helper.highlight(blob_name, blob_content) - - expect(result).to eq(%[
(make-pathname :defaults name\n:type "assem"))
]) - end - - it 'highlights single block' do - expected = %Q[
(make-pathname :defaults name
-:type "assem"))
] - - expect(helper.highlight(blob_name, blob_content)).to eq(expected) - end - - it 'highlights multi-line comments' do - result = helper.highlight(blob_name, multiline_content) - html = Nokogiri::HTML(result) - lines = html.search('.s') - expect(lines.count).to eq(3) - expect(lines[0].text).to eq('"""This is line 1 of a multi-line comment.') - expect(lines[1].text).to eq(' This is line 2.') - expect(lines[2].text).to eq(' """') - end - - context 'diff highlighting' do - let(:blob_name) { 'test.diff' } - let(:blob_content) { "+aaa\n+bbb\n- ccc\n ddd\n"} - let(:expected) do - %q(
+aaa
-+bbb
-- ccc
- ddd
) - end - - it 'highlights each line properly' do - result = helper.highlight(blob_name, blob_content) - expect(result).to eq(expected) - end + it 'handles plain version' do + expect(helper.highlight('test.rb', '52', plain: true)).to eq(%q[
52
]) end end diff --git a/spec/lib/gitlab/highlight_spec.rb b/spec/lib/gitlab/highlight_spec.rb index 77179260a66..fe0e9702f8a 100644 --- a/spec/lib/gitlab/highlight_spec.rb +++ b/spec/lib/gitlab/highlight_spec.rb @@ -18,6 +18,66 @@ describe Gitlab::Highlight do end describe '#highlight' do + let(:file_name) { 'test.lisp' } + let(:no_context_content) { ":type \"assem\"))" } + let(:content) { "(make-pathname :defaults name\n#{no_context_content}" } + let(:multiline_content) do + %q( + def test(input): + """This is line 1 of a multi-line comment. + This is line 2. + """ + ) + end + + it 'highlights' do + expected = %Q[(make-pathname :defaults name +:type "assem"))] + + expect(described_class.highlight(file_name, content)).to eq(expected) + end + + it 'returns plain version for unknown lexer context' do + result = described_class.highlight(file_name, no_context_content) + + expect(result).to eq(%[:type "assem"))]) + end + + it 'returns plain version for long content' do + stub_const('Gitlab::Highlight::MAXIMUM_TEXT_HIGHLIGHT_SIZE', 1) + result = described_class.highlight(file_name, content) + + expect(result).to eq(%[(make-pathname :defaults name\n:type "assem"))]) + end + + it 'highlights multi-line comments' do + result = described_class.highlight(file_name, multiline_content) + html = Nokogiri::HTML(result) + lines = html.search('.s') + + expect(lines.count).to eq(3) + expect(lines[0].text).to eq('"""This is line 1 of a multi-line comment.') + expect(lines[1].text).to eq(' This is line 2.') + expect(lines[2].text).to eq(' """') + end + + context 'diff highlighting' do + let(:file_name) { 'test.diff' } + let(:content) { "+aaa\n+bbb\n- ccc\n ddd\n"} + let(:expected) do + %q(+aaa ++bbb +- ccc + ddd) + end + + it 'highlights each line properly' do + result = described_class.highlight(file_name, content) + + expect(result).to eq(expected) + end + end + describe 'with CRLF' do let(:branch) { 'crlf-diff' } let(:path) { 'files/whitespace' } diff --git a/spec/presenters/blob_presenter_spec.rb b/spec/presenters/blob_presenter_spec.rb index 5c83fa39d80..e85e7a41017 100644 --- a/spec/presenters/blob_presenter_spec.rb +++ b/spec/presenters/blob_presenter_spec.rb @@ -18,31 +18,15 @@ describe BlobPresenter, :seed_helper do subject { described_class.new(blob) } it 'returns highlighted content' do - expect(Gitlab::Highlight).to receive(:highlight).with('files/ruby/regex.rb', git_blob.data, plain: false, language: nil) + expect(Gitlab::Highlight).to receive(:highlight).with('files/ruby/regex.rb', git_blob.data, plain: nil, language: nil) subject.highlight end - context 'with :plain' do - it 'returns plain content when no_highlighting? is true' do - allow(blob).to receive(:no_highlighting?).and_return(true) + it 'returns plain content when :plain is true' do + expect(Gitlab::Highlight).to receive(:highlight).with('files/ruby/regex.rb', git_blob.data, plain: true, language: nil) - subject.highlight - end - - it 'returns plain content when :plain is true' do - expect(Gitlab::Highlight).to receive(:highlight).with('files/ruby/regex.rb', git_blob.data, plain: true, language: nil) - - subject.highlight(plain: true) - end - - it 'returns plain content when :plain is false, but no_highlighting? is true' do - allow(blob).to receive(:no_highlighting?).and_return(true) - - expect(Gitlab::Highlight).to receive(:highlight).with('files/ruby/regex.rb', git_blob.data, plain: true, language: nil) - - subject.highlight(plain: false) - end + subject.highlight(plain: true) end context 'gitlab-language contains a match' do @@ -51,7 +35,7 @@ describe BlobPresenter, :seed_helper do end it 'passes language to inner call' do - expect(Gitlab::Highlight).to receive(:highlight).with('files/ruby/regex.rb', git_blob.data, plain: false, language: 'ruby') + expect(Gitlab::Highlight).to receive(:highlight).with('files/ruby/regex.rb', git_blob.data, plain: nil, language: 'ruby') subject.highlight end