From 860fd922adfba83082aa8e2638250d4bc5d4b65f Mon Sep 17 00:00:00 2001 From: Vasilii Iakliushin Date: Thu, 11 Mar 2021 15:02:31 +0100 Subject: [PATCH] Store MarkerRanges in Gitlab::Diff::Line objects Contibutes to https://gitlab.com/gitlab-org/gitlab/-/issues/16950 Follow-up for https://gitlab.com/gitlab-org/gitlab/-/merge_requests/55669 Extend Gitlab::Diff::Line object with a new `marker_ranges` field. This field keeps information about ranges of characters for each line with the type of the change. Gitlab::Diff::Highlight code was updated to fetch MarkerRanges from Diff::Line objects. --- .../development/use_marker_ranges.yml | 8 ++ lib/gitlab/diff/highlight.rb | 70 +++++++++++++----- lib/gitlab/diff/highlight_cache.rb | 3 +- lib/gitlab/diff/inline_diff.rb | 1 + lib/gitlab/diff/line.rb | 8 +- spec/lib/gitlab/diff/highlight_cache_spec.rb | 14 +++- spec/lib/gitlab/diff/highlight_spec.rb | 20 +++++ spec/lib/gitlab/diff/inline_diff_spec.rb | 74 +++++-------------- spec/lib/gitlab/diff/line_spec.rb | 12 +++ 9 files changed, 130 insertions(+), 80 deletions(-) create mode 100644 config/feature_flags/development/use_marker_ranges.yml diff --git a/config/feature_flags/development/use_marker_ranges.yml b/config/feature_flags/development/use_marker_ranges.yml new file mode 100644 index 00000000000000..068e403e2cf77c --- /dev/null +++ b/config/feature_flags/development/use_marker_ranges.yml @@ -0,0 +1,8 @@ +--- +name: use_marker_ranges +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/56361 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/324638 +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 baa46e7e30608c..23859e2573eb36 100644 --- a/lib/gitlab/diff/highlight.rb +++ b/lib/gitlab/diff/highlight.rb @@ -3,7 +3,7 @@ module Gitlab module Diff class Highlight - attr_reader :diff_file, :diff_lines, :raw_lines, :repository, :project + attr_reader :diff_file, :diff_lines, :repository, :project delegate :old_path, :new_path, :old_sha, :new_sha, to: :diff_file, prefix: :diff @@ -22,29 +22,15 @@ def initialize(diff_lines, repository: nil) end def highlight - @diff_lines.map.with_index do |diff_line, i| + populate_marker_ranges if Feature.enabled?(:use_marker_ranges, project, default_enabled: :yaml) + + @diff_lines.map.with_index do |diff_line, index| diff_line = diff_line.dup # ignore highlighting for "match" lines next diff_line if diff_line.meta? - rich_line = highlight_line(diff_line) || ERB::Util.html_escape(diff_line.text) - - if line_inline_diffs = inline_diffs[i] - begin - # MarkerRange objects are converted to Ranges to keep the previous behavior - # Issue: https://gitlab.com/gitlab-org/gitlab/-/issues/324068 - if Feature.disabled?(:introduce_marker_ranges, project, default_enabled: :yaml) - line_inline_diffs = line_inline_diffs.map { |marker_range| marker_range.to_range } - end - - rich_line = InlineDiffMarker.new(diff_line.text, rich_line).mark(line_inline_diffs) - # This should only happen when the encoding of the diff doesn't - # match the blob, which is a bug. But we shouldn't fail to render - # completely in that case, even though we want to report the error. - rescue RangeError => e - Gitlab::ErrorTracking.track_and_raise_for_dev_exception(e, issue_url: 'https://gitlab.com/gitlab-org/gitlab-foss/issues/45441') - end - end + rich_line = apply_syntax_highlight(diff_line) + rich_line = apply_marker_ranges_highlight(diff_line, rich_line, index) diff_line.rich_text = rich_line @@ -54,6 +40,49 @@ def highlight private + def populate_marker_ranges + pair_selector = Gitlab::Diff::PairSelector.new(@raw_lines) + + pair_selector.each do |old_index, new_index| + old_line = diff_lines[old_index] + new_line = diff_lines[new_index] + + old_diffs, new_diffs = Gitlab::Diff::InlineDiff.new(old_line.text, new_line.text, offset: 1).inline_diffs + + old_line.set_marker_ranges(old_diffs) + new_line.set_marker_ranges(new_diffs) + end + end + + def apply_syntax_highlight(diff_line) + highlight_line(diff_line) || ERB::Util.html_escape(diff_line.text) + end + + def apply_marker_ranges_highlight(diff_line, rich_line, index) + marker_ranges = if Feature.enabled?(:use_marker_ranges, project, default_enabled: :yaml) + diff_line.marker_ranges + else + inline_diffs[index] + end + + return rich_line if marker_ranges.blank? + + begin + # MarkerRange objects are converted to Ranges to keep the previous behavior + # Issue: https://gitlab.com/gitlab-org/gitlab/-/issues/324068 + if Feature.disabled?(:introduce_marker_ranges, project, default_enabled: :yaml) + marker_ranges = marker_ranges.map { |marker_range| marker_range.to_range } + end + + InlineDiffMarker.new(diff_line.text, rich_line).mark(marker_ranges) + # This should only happen when the encoding of the diff doesn't + # match the blob, which is a bug. But we shouldn't fail to render + # completely in that case, even though we want to report the error. + rescue RangeError => e + Gitlab::ErrorTracking.track_and_raise_for_dev_exception(e, issue_url: 'https://gitlab.com/gitlab-org/gitlab-foss/issues/45441') + end + end + def highlight_line(diff_line) return unless diff_file && diff_file.diff_refs @@ -72,6 +101,7 @@ def highlight_line(diff_line) end 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 c5e9bfdc321baa..2192582348c8fc 100644 --- a/lib/gitlab/diff/highlight_cache.rb +++ b/lib/gitlab/diff/highlight_cache.rb @@ -73,7 +73,8 @@ def key 'highlighted-diff-files', diffable.cache_key, VERSION, diff_options, - Feature.enabled?(:introduce_marker_ranges, diffable.project, default_enabled: :yaml) + Feature.enabled?(:introduce_marker_ranges, diffable.project, default_enabled: :yaml), + Feature.enabled?(:use_marker_ranges, diffable.project, default_enabled: :yaml) ].join(":") end end diff --git a/lib/gitlab/diff/inline_diff.rb b/lib/gitlab/diff/inline_diff.rb index dd73e4d6c15de4..f70618195d07db 100644 --- a/lib/gitlab/diff/inline_diff.rb +++ b/lib/gitlab/diff/inline_diff.rb @@ -18,6 +18,7 @@ def inline_diffs CharDiff.new(old_line, new_line).changed_ranges(offset: offset) end + # Deprecated: https://gitlab.com/gitlab-org/gitlab/-/issues/324638 class << self def for_lines(lines) pair_selector = Gitlab::Diff::PairSelector.new(lines) diff --git a/lib/gitlab/diff/line.rb b/lib/gitlab/diff/line.rb index 98ed2400d82ad0..444928b4310f55 100644 --- a/lib/gitlab/diff/line.rb +++ b/lib/gitlab/diff/line.rb @@ -8,7 +8,7 @@ class Line # SERIALIZE_KEYS = %i(line_code rich_text text type index old_pos new_pos).freeze - attr_reader :line_code + attr_reader :line_code, :marker_ranges attr_writer :rich_text attr_accessor :text, :index, :type, :old_pos, :new_pos @@ -21,6 +21,8 @@ def initialize(text, type, index, old_pos, new_pos, parent_file: nil, line_code: # When line code is not provided from cache store we build it # using the parent_file(Diff::File or Conflict::File). @line_code = line_code || calculate_line_code + + @marker_ranges = [] end def self.init_from_hash(hash) @@ -48,6 +50,10 @@ def to_hash hash end + def set_marker_ranges(marker_ranges) + @marker_ranges = marker_ranges + end + def old_line old_pos unless added? || meta? end diff --git a/spec/lib/gitlab/diff/highlight_cache_spec.rb b/spec/lib/gitlab/diff/highlight_cache_spec.rb index d26bc5fc9a856f..8d29b001f8d5b7 100644 --- a/spec/lib/gitlab/diff/highlight_cache_spec.rb +++ b/spec/lib/gitlab/diff/highlight_cache_spec.rb @@ -238,7 +238,7 @@ subject { cache.key } it 'returns cache key' do - is_expected.to eq("highlighted-diff-files:#{cache.diffable.cache_key}:2:#{cache.diff_options}:true") + is_expected.to eq("highlighted-diff-files:#{cache.diffable.cache_key}:2:#{cache.diff_options}:true:true") end context 'when feature flag is disabled' do @@ -247,7 +247,17 @@ 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") + is_expected.to eq("highlighted-diff-files:#{cache.diffable.cache_key}:2:#{cache.diff_options}:false:true") + end + end + + context 'when 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") end end end diff --git a/spec/lib/gitlab/diff/highlight_spec.rb b/spec/lib/gitlab/diff/highlight_spec.rb index e613674af3a0f2..32ca6e4fde6b43 100644 --- a/spec/lib/gitlab/diff/highlight_spec.rb +++ b/spec/lib/gitlab/diff/highlight_spec.rb @@ -65,6 +65,14 @@ expect(subject[5].rich_text).to eq(code) end + + context 'when use_marker_ranges feature flag is false too' do + it 'does not affect the result' do + code = %Q{+ raise RuntimeError, "System commands must be given as an array of strings"\n} + + expect(subject[5].rich_text).to eq(code) + end + end end context 'when no diff_refs' do @@ -132,6 +140,18 @@ end end + context 'when `use_marker_ranges` feature flag is disabled' do + it 'returns the same result' do + with_feature_flag = described_class.new(diff_file, repository: project.repository).highlight + + stub_feature_flags(use_marker_ranges: false) + + without_feature_flag = described_class.new(diff_file, repository: project.repository).highlight + + expect(with_feature_flag.map(&:rich_text)).to eq(without_feature_flag.map(&:rich_text)) + end + end + context 'when no inline diffs' do it_behaves_like 'without inline diffs' end diff --git a/spec/lib/gitlab/diff/inline_diff_spec.rb b/spec/lib/gitlab/diff/inline_diff_spec.rb index 714b5d813c475e..d7b50eb73ee9e0 100644 --- a/spec/lib/gitlab/diff/inline_diff_spec.rb +++ b/spec/lib/gitlab/diff/inline_diff_spec.rb @@ -3,68 +3,30 @@ require 'spec_helper' RSpec.describe Gitlab::Diff::InlineDiff do - describe '.for_lines' do - let(:diff) do - <<-EOF.strip_heredoc - class Test - - def initialize(test = true) - + def initialize(test = false) - @test = test - - if true - - @foo = "bar" - + unless false - + @foo = "baz" - end - end - end - EOF - end - - let(:subject) { described_class.for_lines(diff.lines) } + describe '#inline_diffs' do + subject { described_class.new(old_line, new_line, offset: offset).inline_diffs } - it 'finds all inline diffs' do - expect(subject[0]).to be_nil - expect(subject[1]).to eq([25..27]) - expect(subject[2]).to eq([25..28]) - expect(subject[3]).to be_nil - expect(subject[4]).to eq([5..10]) - expect(subject[5]).to eq([17..17]) - expect(subject[6]).to eq([5..15]) - expect(subject[7]).to eq([17..17]) - expect(subject[8]).to be_nil - end + let(:old_line) { 'XXX def initialize(test = true)' } + let(:new_line) { 'YYY def initialize(test = false)' } + let(:offset) { 3 } - it 'can handle unchanged empty lines' do - expect { described_class.for_lines(['- bar', '+ baz', '']) }.not_to raise_error + it 'finds the inline diff', :aggregate_failures do + expect(subject[0]).to eq([Gitlab::MarkerRange.new(26, 28, mode: :deletion)]) + expect(subject[1]).to eq([Gitlab::MarkerRange.new(26, 29, mode: :addition)]) end context 'when lines have multiple changes' do - let(:diff) do - <<~EOF - - Hello, how are you? - + Hi, how are you doing? - EOF - end - - let(:subject) { described_class.for_lines(diff.lines) } - - it 'finds all inline diffs' do - expect(subject[0]).to eq([3..6]) - expect(subject[1]).to eq([3..3, 17..22]) + let(:old_line) { '- Hello, how are you?' } + let(:new_line) { '+ Hi, how are you doing?' } + let(:offset) { 1 } + + it 'finds all inline diffs', :aggregate_failures do + expect(subject[0]).to eq([Gitlab::MarkerRange.new(3, 6, mode: :deletion)]) + expect(subject[1]).to eq([ + Gitlab::MarkerRange.new(3, 3, mode: :addition), + Gitlab::MarkerRange.new(17, 22, mode: :addition) + ]) end end end - - describe "#inline_diffs" do - let(:old_line) { "XXX def initialize(test = true)" } - let(:new_line) { "YYY def initialize(test = false)" } - let(:subject) { described_class.new(old_line, new_line, offset: 3).inline_diffs } - - it "finds the inline diff" do - old_diffs, new_diffs = subject - - expect(old_diffs).to eq([26..28]) - expect(new_diffs).to eq([26..29]) - end - end end diff --git a/spec/lib/gitlab/diff/line_spec.rb b/spec/lib/gitlab/diff/line_spec.rb index e10a50afde948c..a40cd99f6f86e8 100644 --- a/spec/lib/gitlab/diff/line_spec.rb +++ b/spec/lib/gitlab/diff/line_spec.rb @@ -17,6 +17,8 @@ rich_text: rich_text) end + let(:rich_text) { nil } + describe '.init_from_hash' do let(:rich_text) { '<input>' } @@ -51,4 +53,14 @@ expect(line[:rich_text]).to eq("<input>") end end + + describe '#set_marker_ranges' do + let(:marker_ranges) { [Gitlab::MarkerRange.new(1, 10, mode: :deletion)] } + + it 'stores MarkerRanges in Diff::Line object' do + line.set_marker_ranges(marker_ranges) + + expect(line.marker_ranges).to eq(marker_ranges) + end + end end -- GitLab