From d32be0320bdd29c945fe3e776688387b6dbb530d Mon Sep 17 00:00:00 2001 From: Vasilii Iakliushin Date: Tue, 9 Mar 2021 12:02:32 +0100 Subject: [PATCH] Update syntax highlighting logic Contributes to https://gitlab.com/gitlab-org/gitlab/-/issues/16950 FF issue: https://gitlab.com/gitlab-org/gitlab/-/issues/324159 We apply syntax highlighting to old and new versions of the whole file. However, we need to highlight only diff lines. This change should optimize this process. Here we provide only necessary diff lines to the syntax highlighter. --- .../diff_line_syntax_highlighting.yml | 8 ++++ lib/gitlab/diff/highlight.rb | 36 +++++++++++++++++ lib/gitlab/diff/highlight_cache.rb | 6 ++- lib/gitlab/diff/line.rb | 10 ++++- lib/gitlab/highlight.rb | 10 +++-- lib/rouge/formatters/html_gitlab.rb | 5 ++- spec/lib/gitlab/diff/highlight_cache_spec.rb | 20 +++++++--- spec/lib/gitlab/diff/line_spec.rb | 23 +++++++++++ spec/lib/gitlab/highlight_spec.rb | 15 +++++++ spec/lib/rouge/formatters/html_gitlab_spec.rb | 40 +++++++++++++++++++ 10 files changed, 159 insertions(+), 14 deletions(-) create mode 100644 config/feature_flags/development/diff_line_syntax_highlighting.yml create mode 100644 spec/lib/rouge/formatters/html_gitlab_spec.rb diff --git a/config/feature_flags/development/diff_line_syntax_highlighting.yml b/config/feature_flags/development/diff_line_syntax_highlighting.yml new file mode 100644 index 00000000000000..3244dad6c24108 --- /dev/null +++ b/config/feature_flags/development/diff_line_syntax_highlighting.yml @@ -0,0 +1,8 @@ +--- +name: diff_line_syntax_highlighting +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/56108 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/324159 +milestone: '13.10' +type: development +group: group::source code +default_enabled: false diff --git a/lib/gitlab/diff/highlight.rb b/lib/gitlab/diff/highlight.rb index 23859e2573eb36..8385bbbb3ded59 100644 --- a/lib/gitlab/diff/highlight.rb +++ b/lib/gitlab/diff/highlight.rb @@ -86,6 +86,41 @@ def apply_marker_ranges_highlight(diff_line, rich_line, index) def highlight_line(diff_line) return unless diff_file && diff_file.diff_refs + if Feature.enabled?(:diff_line_syntax_highlighting, project, default_enabled: :yaml) + diff_line_highlighting(diff_line) + else + blob_highlighting(diff_line) + end + end + + def diff_line_highlighting(diff_line) + rich_line = syntax_highlighter(diff_line).highlight( + diff_line.text(prefix: false), + context: { line_number: diff_line.line } + )&.html_safe + + # Only update text if line is found. This will prevent + # issues with submodules given the line only exists in diff content. + if rich_line + line_prefix = diff_line.text =~ /\A(.)/ ? Regexp.last_match(1) : ' ' + rich_line.prepend(line_prefix).concat("\n") + end + end + + def syntax_highlighter(diff_line) + path = diff_line.removed? ? diff_file.old_path : diff_file.new_path + + @syntax_highlighter ||= {} + @syntax_highlighter[path] ||= Gitlab::Highlight.new( + path, + @raw_lines, + language: repository&.gitattribute(path, 'gitlab-language') + ) + end + + # Deprecated: https://gitlab.com/gitlab-org/gitlab/-/issues/324159 + # ------------------------------------------------------------------------ + def blob_highlighting(diff_line) rich_line = if diff_line.unchanged? || diff_line.added? new_lines[diff_line.new_pos - 1]&.html_safe @@ -102,6 +137,7 @@ def highlight_line(diff_line) end # Deprecated: https://gitlab.com/gitlab-org/gitlab/-/issues/324638 + # ------------------------------------------------------------------------ def inline_diffs @inline_diffs ||= InlineDiff.for_lines(@raw_lines) end diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb index 2192582348c8fc..209462fd6e9dd0 100644 --- a/lib/gitlab/diff/highlight_cache.rb +++ b/lib/gitlab/diff/highlight_cache.rb @@ -71,10 +71,12 @@ def key strong_memoize(:redis_key) do [ 'highlighted-diff-files', - diffable.cache_key, VERSION, + diffable.cache_key, + VERSION, diff_options, Feature.enabled?(:introduce_marker_ranges, diffable.project, default_enabled: :yaml), - Feature.enabled?(:use_marker_ranges, diffable.project, default_enabled: :yaml) + Feature.enabled?(:use_marker_ranges, diffable.project, default_enabled: :yaml), + Feature.enabled?(:diff_line_syntax_highlighting, diffable.project, default_enabled: :yaml) ].join(":") end end diff --git a/lib/gitlab/diff/line.rb b/lib/gitlab/diff/line.rb index 444928b4310f55..66f506ec3aa1f8 100644 --- a/lib/gitlab/diff/line.rb +++ b/lib/gitlab/diff/line.rb @@ -9,8 +9,8 @@ class Line SERIALIZE_KEYS = %i(line_code rich_text text type index old_pos new_pos).freeze attr_reader :line_code, :marker_ranges - attr_writer :rich_text - attr_accessor :text, :index, :type, :old_pos, :new_pos + attr_writer :text, :rich_text + attr_accessor :index, :type, :old_pos, :new_pos def initialize(text, type, index, old_pos, new_pos, parent_file: nil, line_code: nil, rich_text: nil) @text, @type, @index = text, type, index @@ -54,6 +54,12 @@ def set_marker_ranges(marker_ranges) @marker_ranges = marker_ranges end + def text(prefix: true) + return @text if prefix + + @text&.slice(1..).to_s + end + def old_line old_pos unless added? || meta? end diff --git a/lib/gitlab/highlight.rb b/lib/gitlab/highlight.rb index 40dee0142b9844..e4527f06eff2ef 100644 --- a/lib/gitlab/highlight.rb +++ b/lib/gitlab/highlight.rb @@ -20,7 +20,9 @@ def initialize(blob_name, blob_content, language: nil) @blob_content = blob_content end - def highlight(text, continue: true, plain: false) + def highlight(text, continue: false, plain: false, context: {}) + @context = context + plain ||= text.length > MAXIMUM_TEXT_HIGHLIGHT_SIZE highlighted_text = highlight_text(text, continue: continue, plain: plain) @@ -38,6 +40,8 @@ def lexer private + attr_reader :context + def custom_language return unless @language @@ -53,13 +57,13 @@ def highlight_text(text, continue: true, plain: false) end def highlight_plain(text) - @formatter.format(Rouge::Lexers::PlainText.lex(text)).html_safe + @formatter.format(Rouge::Lexers::PlainText.lex(text), context).html_safe end def highlight_rich(text, continue: true) tag = lexer.tag tokens = lexer.lex(text, continue: continue) - Timeout.timeout(timeout_time) { @formatter.format(tokens, tag: tag).html_safe } + Timeout.timeout(timeout_time) { @formatter.format(tokens, context.merge(tag: tag)).html_safe } rescue Timeout::Error => e Gitlab::ErrorTracking.track_and_raise_for_dev_exception(e) highlight_plain(text) diff --git a/lib/rouge/formatters/html_gitlab.rb b/lib/rouge/formatters/html_gitlab.rb index 8f18d6433e00bf..e0e9677fac7d80 100644 --- a/lib/rouge/formatters/html_gitlab.rb +++ b/lib/rouge/formatters/html_gitlab.rb @@ -7,10 +7,11 @@ class HTMLGitlab < Rouge::Formatters::HTML # Creates a new Rouge::Formatter::HTMLGitlab instance. # - # [+tag+] The tag (language) of the lexer used to generate the formatted tokens + # [+tag+] The tag (language) of the lexer used to generate the formatted tokens + # [+line_number+] The line number used to populate line IDs def initialize(options = {}) - @line_number = 1 @tag = options[:tag] + @line_number = options[:line_number] || 1 end def stream(tokens) diff --git a/spec/lib/gitlab/diff/highlight_cache_spec.rb b/spec/lib/gitlab/diff/highlight_cache_spec.rb index 8d29b001f8d5b7..4c56911e665218 100644 --- a/spec/lib/gitlab/diff/highlight_cache_spec.rb +++ b/spec/lib/gitlab/diff/highlight_cache_spec.rb @@ -238,26 +238,36 @@ subject { cache.key } it 'returns cache key' do - is_expected.to eq("highlighted-diff-files:#{cache.diffable.cache_key}:2:#{cache.diff_options}:true:true") + is_expected.to eq("highlighted-diff-files:#{cache.diffable.cache_key}:2:#{cache.diff_options}:true:true:true") end - context 'when feature flag is disabled' do + context 'when the `introduce_marker_ranges` feature flag is disabled' do before do stub_feature_flags(introduce_marker_ranges: false) end it 'returns the original version of the cache' do - is_expected.to eq("highlighted-diff-files:#{cache.diffable.cache_key}:2:#{cache.diff_options}:false:true") + is_expected.to eq("highlighted-diff-files:#{cache.diffable.cache_key}:2:#{cache.diff_options}:false:true:true") end end - context 'when use marker ranges feature flag is disabled' do + context 'when the `use_marker_ranges` feature flag is disabled' do before do stub_feature_flags(use_marker_ranges: false) end it 'returns the original version of the cache' do - is_expected.to eq("highlighted-diff-files:#{cache.diffable.cache_key}:2:#{cache.diff_options}:true:false") + is_expected.to eq("highlighted-diff-files:#{cache.diffable.cache_key}:2:#{cache.diff_options}:true:false:true") + end + end + + context 'when the `diff_line_syntax_highlighting` feature flag is disabled' do + before do + stub_feature_flags(diff_line_syntax_highlighting: false) + end + + it 'returns the original version of the cache' do + is_expected.to eq("highlighted-diff-files:#{cache.diffable.cache_key}:2:#{cache.diff_options}:true:true:false") end end end diff --git a/spec/lib/gitlab/diff/line_spec.rb b/spec/lib/gitlab/diff/line_spec.rb index a40cd99f6f86e8..949def599ae6a3 100644 --- a/spec/lib/gitlab/diff/line_spec.rb +++ b/spec/lib/gitlab/diff/line_spec.rb @@ -45,6 +45,29 @@ end end + describe '#text' do + let(:line) { described_class.new(raw_diff, 'new', 0, 0, 0) } + let(:raw_diff) { '+Hello' } + + it 'returns raw diff text' do + expect(line.text).to eq('+Hello') + end + + context 'when prefix is disabled' do + it 'returns raw diff text without prefix' do + expect(line.text(prefix: false)).to eq('Hello') + end + + context 'when diff is empty' do + let(:raw_diff) { '' } + + it 'returns an empty raw diff' do + expect(line.text(prefix: false)).to eq('') + end + end + end + end + context "when setting rich text" do it 'escapes any HTML special characters in the diff chunk header' do subject = described_class.new("", "", 0, 0, 0) diff --git a/spec/lib/gitlab/highlight_spec.rb b/spec/lib/gitlab/highlight_spec.rb index 9271b868e36073..1a929373716acd 100644 --- a/spec/lib/gitlab/highlight_spec.rb +++ b/spec/lib/gitlab/highlight_spec.rb @@ -79,6 +79,21 @@ def test(input): expect(result).to eq(expected) end + + context 'when start line number is set' do + let(:expected) do + %q(+aaa ++bbb +- ccc + ddd) + end + + it 'highlights each line properly' do + result = described_class.new(file_name, content).highlight(content, context: { line_number: 10 }) + + expect(result).to eq(expected) + end + end end describe 'with CRLF' do diff --git a/spec/lib/rouge/formatters/html_gitlab_spec.rb b/spec/lib/rouge/formatters/html_gitlab_spec.rb new file mode 100644 index 00000000000000..d45c8c2a8c5674 --- /dev/null +++ b/spec/lib/rouge/formatters/html_gitlab_spec.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Rouge::Formatters::HTMLGitlab do + describe '#format' do + subject { described_class.format(tokens, options) } + + let(:lang) { 'ruby' } + let(:lexer) { Rouge::Lexer.find_fancy(lang) } + let(:tokens) { lexer.lex("def hello", continue: false) } + let(:options) { { tag: lang } } + + it 'returns highlighted ruby code' do + code = %q{def hello} + + is_expected.to eq(code) + end + + context 'when options are empty' do + let(:options) { {} } + + it 'returns highlighted code without language' do + code = %q{def hello} + + is_expected.to eq(code) + end + end + + context 'when line number is provided' do + let(:options) { { tag: lang, line_number: 10 } } + + it 'returns highlighted ruby code with correct line number' do + code = %q{def hello} + + is_expected.to eq(code) + end + end + end +end -- GitLab