From 2b68d4d3dd4803382f058b82fb3cbca82eb234d7 Mon Sep 17 00:00:00 2001 From: Vasilii Iakliushin Date: Mon, 1 Mar 2021 16:41:09 +0100 Subject: [PATCH 1/3] Introduce MarkerRange class Contributes to https://gitlab.com/gitlab-org/gitlab/-/issues/16950 MarkerRange behaves like a Range but can additionally keep information about the type of the change. It helps to identify if changed characters were removed or added. --- app/helpers/diff_helper.rb | 4 +- .../system_notes/issuables_service.rb | 4 +- lib/banzai/filter/autolink_filter.rb | 2 +- lib/banzai/filter/spaced_link_filter.rb | 2 +- lib/gitlab/dependency_linker/base_linker.rb | 2 +- lib/gitlab/dependency_linker/go_mod_linker.rb | 2 +- lib/gitlab/dependency_linker/go_sum_linker.rb | 2 +- lib/gitlab/diff/char_diff.rb | 4 +- .../diff/inline_diff_markdown_marker.rb | 4 +- lib/gitlab/diff/inline_diff_marker.rb | 4 +- lib/gitlab/marker_range.rb | 25 ++++++++ lib/gitlab/string_range_marker.rb | 18 +++--- spec/lib/gitlab/diff/highlight_spec.rb | 4 +- .../diff/inline_diff_markdown_marker_spec.rb | 4 +- spec/lib/gitlab/marker_range_spec.rb | 57 +++++++++++++++++++ spec/lib/gitlab/string_range_marker_spec.rb | 2 +- spec/lib/gitlab/string_regex_marker_spec.rb | 4 +- 17 files changed, 114 insertions(+), 30 deletions(-) create mode 100644 lib/gitlab/marker_range.rb create mode 100644 spec/lib/gitlab/marker_range_spec.rb diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index aa65ba95c2e79d..10c7b4032cf938 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -4,8 +4,8 @@ module DiffHelper def mark_inline_diffs(old_line, new_line) old_diffs, new_diffs = Gitlab::Diff::InlineDiff.new(old_line, new_line).inline_diffs - marked_old_line = Gitlab::Diff::InlineDiffMarker.new(old_line).mark(old_diffs, mode: :deletion) - marked_new_line = Gitlab::Diff::InlineDiffMarker.new(new_line).mark(new_diffs, mode: :addition) + marked_old_line = Gitlab::Diff::InlineDiffMarker.new(old_line).mark(old_diffs) + marked_new_line = Gitlab::Diff::InlineDiffMarker.new(new_line).mark(new_diffs) [marked_old_line, marked_new_line] end diff --git a/app/services/system_notes/issuables_service.rb b/app/services/system_notes/issuables_service.rb index 69008c702d51b8..60dd56e772ace7 100644 --- a/app/services/system_notes/issuables_service.rb +++ b/app/services/system_notes/issuables_service.rb @@ -125,8 +125,8 @@ def change_title(old_title) old_diffs, new_diffs = Gitlab::Diff::InlineDiff.new(old_title, new_title).inline_diffs - marked_old_title = Gitlab::Diff::InlineDiffMarkdownMarker.new(old_title).mark(old_diffs, mode: :deletion) - marked_new_title = Gitlab::Diff::InlineDiffMarkdownMarker.new(new_title).mark(new_diffs, mode: :addition) + marked_old_title = Gitlab::Diff::InlineDiffMarkdownMarker.new(old_title).mark(old_diffs) + marked_new_title = Gitlab::Diff::InlineDiffMarkdownMarker.new(new_title).mark(new_diffs) body = "changed title from **#{marked_old_title}** to **#{marked_new_title}**" diff --git a/lib/banzai/filter/autolink_filter.rb b/lib/banzai/filter/autolink_filter.rb index 0aa1ee8f604c62..d569711431c27b 100644 --- a/lib/banzai/filter/autolink_filter.rb +++ b/lib/banzai/filter/autolink_filter.rb @@ -120,7 +120,7 @@ def autolink_match(match) end def autolink_filter(text) - Gitlab::StringRegexMarker.new(CGI.unescapeHTML(text), text.html_safe).mark(LINK_PATTERN) do |link, left:, right:| + Gitlab::StringRegexMarker.new(CGI.unescapeHTML(text), text.html_safe).mark(LINK_PATTERN) do |link, left:, right:, mode:| autolink_match(link).html_safe end end diff --git a/lib/banzai/filter/spaced_link_filter.rb b/lib/banzai/filter/spaced_link_filter.rb index 101b55a49e4090..f52ffe117d9b38 100644 --- a/lib/banzai/filter/spaced_link_filter.rb +++ b/lib/banzai/filter/spaced_link_filter.rb @@ -76,7 +76,7 @@ def spaced_link_match(link) end def spaced_link_filter(text) - Gitlab::StringRegexMarker.new(CGI.unescapeHTML(text), text.html_safe).mark(LINK_OR_IMAGE_PATTERN) do |link, left:, right:| + Gitlab::StringRegexMarker.new(CGI.unescapeHTML(text), text.html_safe).mark(LINK_OR_IMAGE_PATTERN) do |link, left:, right:, mode:| spaced_link_match(link).html_safe end end diff --git a/lib/gitlab/dependency_linker/base_linker.rb b/lib/gitlab/dependency_linker/base_linker.rb index d735fb55652d91..36a840372c5a26 100644 --- a/lib/gitlab/dependency_linker/base_linker.rb +++ b/lib/gitlab/dependency_linker/base_linker.rb @@ -80,7 +80,7 @@ def link_regex(regex, &url_proc) highlighted_lines.map!.with_index do |rich_line, i| marker = StringRegexMarker.new((plain_lines[i].chomp! || plain_lines[i]), rich_line.html_safe) - marker.mark(regex, group: :name) do |text, left:, right:| + marker.mark(regex, group: :name) do |text, left:, right:, mode:| url = yield(text) url ? link_tag(text, url) : text end diff --git a/lib/gitlab/dependency_linker/go_mod_linker.rb b/lib/gitlab/dependency_linker/go_mod_linker.rb index 4d6fe366333e3b..fae4ee23383de8 100644 --- a/lib/gitlab/dependency_linker/go_mod_linker.rb +++ b/lib/gitlab/dependency_linker/go_mod_linker.rb @@ -22,7 +22,7 @@ def link_dependencies i, j = match.offset(:name) marker = StringRangeMarker.new(plain_line, rich_line.html_safe) - marker.mark([i..(j - 1)]) do |text, left:, right:| + marker.mark([i..(j - 1)]) do |text, left:, right:, mode:| url = package_url(text, match[:version]) url ? link_tag(text, url) : text end diff --git a/lib/gitlab/dependency_linker/go_sum_linker.rb b/lib/gitlab/dependency_linker/go_sum_linker.rb index 20dc82ede9f45a..44826332f66de3 100644 --- a/lib/gitlab/dependency_linker/go_sum_linker.rb +++ b/lib/gitlab/dependency_linker/go_sum_linker.rb @@ -21,7 +21,7 @@ def link_dependencies i2, j2 = match.offset(:checksum) marker = StringRangeMarker.new(plain_line, rich_line.html_safe) - marker.mark([i0..(j0 - 1), i2..(j2 - 1)]) do |text, left:, right:| + marker.mark([i0..(j0 - 1), i2..(j2 - 1)]) do |text, left:, right:, mode:| if left url = package_url(text, match[:version]) url ? link_tag(text, url) : text diff --git a/lib/gitlab/diff/char_diff.rb b/lib/gitlab/diff/char_diff.rb index c8bb39e9f5dede..1b3af8f75ca543 100644 --- a/lib/gitlab/diff/char_diff.rb +++ b/lib/gitlab/diff/char_diff.rb @@ -32,12 +32,12 @@ def changed_ranges(offset: 0) end if action == :delete - old_diffs << (old_pointer..(old_pointer + content_size - 1)) + old_diffs << MarkerRange.new(old_pointer, old_pointer + content_size - 1, mode: MarkerRange::DELETION) old_pointer += content_size end if action == :insert - new_diffs << (new_pointer..(new_pointer + content_size - 1)) + new_diffs << MarkerRange.new(new_pointer, new_pointer + content_size - 1, mode: MarkerRange::ADDITION) new_pointer += content_size end end diff --git a/lib/gitlab/diff/inline_diff_markdown_marker.rb b/lib/gitlab/diff/inline_diff_markdown_marker.rb index 3c536c43a9e302..d8d596ebce71e7 100644 --- a/lib/gitlab/diff/inline_diff_markdown_marker.rb +++ b/lib/gitlab/diff/inline_diff_markdown_marker.rb @@ -8,8 +8,8 @@ class InlineDiffMarkdownMarker < Gitlab::StringRangeMarker deletion: "-" }.freeze - def mark(line_inline_diffs, mode: nil) - super(line_inline_diffs) do |text, left:, right:| + def mark(line_inline_diffs) + super(line_inline_diffs) do |text, left:, right:, mode:| symbol = MARKDOWN_SYMBOLS[mode] "{#{symbol}#{text}#{symbol}}" end diff --git a/lib/gitlab/diff/inline_diff_marker.rb b/lib/gitlab/diff/inline_diff_marker.rb index 29dff699ba51e5..c8cc1c0e649b34 100644 --- a/lib/gitlab/diff/inline_diff_marker.rb +++ b/lib/gitlab/diff/inline_diff_marker.rb @@ -7,8 +7,8 @@ def initialize(line, rich_line = nil) super(line, rich_line || line) end - def mark(line_inline_diffs, mode: nil) - super(line_inline_diffs) do |text, left:, right:| + def mark(line_inline_diffs) + super(line_inline_diffs) do |text, left:, right:, mode:| %{#{text}}.html_safe end end diff --git a/lib/gitlab/marker_range.rb b/lib/gitlab/marker_range.rb new file mode 100644 index 00000000000000..d00d7bb1557a32 --- /dev/null +++ b/lib/gitlab/marker_range.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +# It is a Range object extended with `mode` attribute +# MarkerRange not only keeps information about changed characters, but also +# the type of changes +module Gitlab + class MarkerRange < Range + DELETION = :deletion + ADDITION = :addition + + # Converts Range object to MarkerRange class + def self.from_range(range) + return range if range.is_a?(self) + + new(range.begin, range.end, exclude_end: range.exclude_end?) + end + + def initialize(first, last, exclude_end: false, mode: nil) + super(first, last, exclude_end) + @mode = mode + end + + attr_reader :mode + end +end diff --git a/lib/gitlab/string_range_marker.rb b/lib/gitlab/string_range_marker.rb index 780fe4c77255bd..5ddc88edf5036d 100644 --- a/lib/gitlab/string_range_marker.rb +++ b/lib/gitlab/string_range_marker.rb @@ -15,8 +15,10 @@ def initialize(raw_line, rich_line = nil) end end - def mark(marker_ranges) - return rich_line unless marker_ranges&.any? + def mark(ranges) + return rich_line unless ranges&.any? + + marker_ranges = ranges.map { |range| Gitlab::MarkerRange.from_range(range) } if html_escaped rich_marker_ranges = [] @@ -24,7 +26,7 @@ def mark(marker_ranges) # Map the inline-diff range based on the raw line to character positions in the rich line rich_positions = position_mapping[range].flatten # Turn the array of character positions into ranges - rich_marker_ranges.concat(collapse_ranges(rich_positions)) + rich_marker_ranges.concat(collapse_ranges(rich_positions, range.mode)) end else rich_marker_ranges = marker_ranges @@ -36,7 +38,7 @@ def mark(marker_ranges) offset_range = (range.begin + offset)..(range.end + offset) original_text = rich_line[offset_range] - text = yield(original_text, left: i == 0, right: i == rich_marker_ranges.length - 1) + text = yield(original_text, left: i == 0, right: i == rich_marker_ranges.length - 1, mode: range.mode) rich_line[offset_range] = text @@ -90,21 +92,21 @@ def position_mapping end # Takes an array of integers, and returns an array of ranges covering the same integers - def collapse_ranges(positions) + def collapse_ranges(positions, mode) return [] if positions.empty? ranges = [] start = prev = positions[0] - range = start..prev + range = MarkerRange.new(start, prev, mode: mode) positions[1..-1].each do |pos| if pos == prev + 1 - range = start..pos + range = MarkerRange.new(start, pos, mode: mode) prev = pos else ranges << range start = prev = pos - range = start..prev + range = MarkerRange.new(start, prev, mode: mode) end end ranges << range diff --git a/spec/lib/gitlab/diff/highlight_spec.rb b/spec/lib/gitlab/diff/highlight_spec.rb index 283437e7fbd546..c78048e6a96a35 100644 --- a/spec/lib/gitlab/diff/highlight_spec.rb +++ b/spec/lib/gitlab/diff/highlight_spec.rb @@ -50,7 +50,7 @@ end it 'highlights and marks added lines' do - code = %Q{+ raise RuntimeError, "System commands must be given as an array of strings"\n} + code = %Q{+ raise RuntimeError, "System commands must be given as an array of strings"\n} expect(subject[5].rich_text).to eq(code) end @@ -93,7 +93,7 @@ end it 'marks added lines' do - code = %q{+ raise RuntimeError, "System commands must be given as an array of strings"} + code = %q{+ raise RuntimeError, "System commands must be given as an array of strings"} expect(subject[5].rich_text).to eq(code) expect(subject[5].rich_text).to be_html_safe diff --git a/spec/lib/gitlab/diff/inline_diff_markdown_marker_spec.rb b/spec/lib/gitlab/diff/inline_diff_markdown_marker_spec.rb index 60f7f3a103f097..3670074cc21432 100644 --- a/spec/lib/gitlab/diff/inline_diff_markdown_marker_spec.rb +++ b/spec/lib/gitlab/diff/inline_diff_markdown_marker_spec.rb @@ -5,8 +5,8 @@ RSpec.describe Gitlab::Diff::InlineDiffMarkdownMarker do describe '#mark' do let(:raw) { "abc 'def'" } - let(:inline_diffs) { [2..5] } - let(:subject) { described_class.new(raw).mark(inline_diffs, mode: :deletion) } + let(:inline_diffs) { [Gitlab::MarkerRange.new(2, 5, mode: Gitlab::MarkerRange::DELETION)] } + let(:subject) { described_class.new(raw).mark(inline_diffs) } it 'does not escape html etities and marks the range' do expect(subject).to eq("ab{-c 'd-}ef'") diff --git a/spec/lib/gitlab/marker_range_spec.rb b/spec/lib/gitlab/marker_range_spec.rb new file mode 100644 index 00000000000000..4935e01370f4e2 --- /dev/null +++ b/spec/lib/gitlab/marker_range_spec.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' + +RSpec.describe Gitlab::MarkerRange do + subject(:marker_range) { described_class.new(first, last, mode: mode) } + + let(:first) { 1 } + let(:last) { 10 } + let(:mode) { nil } + + it { is_expected.to eq(first..last) } + + it 'behaves like a Range' do + is_expected.to be_kind_of(Range) + end + + describe '#mode' do + subject { marker_range.mode } + + it { is_expected.to be_nil } + + context 'when mode is provided' do + let(:mode) { :deletion } + + it { is_expected.to eq(mode) } + end + end + + describe '.from_range' do + subject { described_class.from_range(range) } + + let(:range) { 1..3 } + + it 'converts Range to MarkerRange object' do + is_expected.to be_a(described_class) + end + + it 'keeps correct range' do + is_expected.to eq(range) + end + + context 'when range excludes end' do + let(:range) { 1...3 } + + it 'keeps correct range' do + is_expected.to eq(range) + end + end + + context 'when range is already a MarkerRange' do + let(:range) { marker_range } + + it { is_expected.to be(marker_range) } + end + end +end diff --git a/spec/lib/gitlab/string_range_marker_spec.rb b/spec/lib/gitlab/string_range_marker_spec.rb index 52fab6e3109efb..6f63c8e2df4066 100644 --- a/spec/lib/gitlab/string_range_marker_spec.rb +++ b/spec/lib/gitlab/string_range_marker_spec.rb @@ -8,7 +8,7 @@ def mark_diff(rich = nil) raw = 'abc ' inline_diffs = [2..5] - described_class.new(raw, rich).mark(inline_diffs) do |text, left:, right:| + described_class.new(raw, rich).mark(inline_diffs) do |text, left:, right:, mode:| "LEFT#{text}RIGHT".html_safe end end diff --git a/spec/lib/gitlab/string_regex_marker_spec.rb b/spec/lib/gitlab/string_regex_marker_spec.rb index 2dadd222820d56..a02be83558ca11 100644 --- a/spec/lib/gitlab/string_regex_marker_spec.rb +++ b/spec/lib/gitlab/string_regex_marker_spec.rb @@ -9,7 +9,7 @@ let(:rich) { %{"name": "AFNetworking"}.html_safe } subject do - described_class.new(raw, rich).mark(/"[^"]+":\s*"(?[^"]+)"/, group: :name) do |text, left:, right:| + described_class.new(raw, rich).mark(/"[^"]+":\s*"(?[^"]+)"/, group: :name) do |text, left:, right:, mode:| %{#{text}}.html_safe end end @@ -25,7 +25,7 @@ let(:rich) { %{a <b> <c> d}.html_safe } subject do - described_class.new(raw, rich).mark(/<[a-z]>/) do |text, left:, right:| + described_class.new(raw, rich).mark(/<[a-z]>/) do |text, left:, right:, mode:| %{#{text}}.html_safe end end -- GitLab From 8a617d71154606ce3cb6062a676e15bccf59790d Mon Sep 17 00:00:00 2001 From: Vasilii Iakliushin Date: Wed, 10 Mar 2021 17:14:44 +0100 Subject: [PATCH 2/3] Add a feature flag for MarkerRanges Issue: https://gitlab.com/gitlab-org/gitlab/-/issues/324068 --- .../development/introduce_marker_ranges.yml | 8 ++++++++ lib/gitlab/diff/highlight.rb | 9 ++++++++- lib/gitlab/marker_range.rb | 4 ++++ spec/lib/gitlab/diff/highlight_spec.rb | 12 ++++++++++++ spec/lib/gitlab/marker_range_spec.rb | 14 ++++++++++++++ 5 files changed, 46 insertions(+), 1 deletion(-) create mode 100644 config/feature_flags/development/introduce_marker_ranges.yml diff --git a/config/feature_flags/development/introduce_marker_ranges.yml b/config/feature_flags/development/introduce_marker_ranges.yml new file mode 100644 index 00000000000000..de59cf0e906767 --- /dev/null +++ b/config/feature_flags/development/introduce_marker_ranges.yml @@ -0,0 +1,8 @@ +--- +name: introduce_marker_ranges +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/55669 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/324068 +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 a5259079345232..baa46e7e30608c 100644 --- a/lib/gitlab/diff/highlight.rb +++ b/lib/gitlab/diff/highlight.rb @@ -3,12 +3,13 @@ module Gitlab module Diff class Highlight - attr_reader :diff_file, :diff_lines, :raw_lines, :repository + attr_reader :diff_file, :diff_lines, :raw_lines, :repository, :project delegate :old_path, :new_path, :old_sha, :new_sha, to: :diff_file, prefix: :diff def initialize(diff_lines, repository: nil) @repository = repository + @project = repository&.project if diff_lines.is_a?(Gitlab::Diff::File) @diff_file = diff_lines @@ -30,6 +31,12 @@ def highlight 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 diff --git a/lib/gitlab/marker_range.rb b/lib/gitlab/marker_range.rb index d00d7bb1557a32..50a59adebdf1ea 100644 --- a/lib/gitlab/marker_range.rb +++ b/lib/gitlab/marker_range.rb @@ -20,6 +20,10 @@ def initialize(first, last, exclude_end: false, mode: nil) @mode = mode end + def to_range + Range.new(self.begin, self.end, self.exclude_end?) + end + attr_reader :mode end end diff --git a/spec/lib/gitlab/diff/highlight_spec.rb b/spec/lib/gitlab/diff/highlight_spec.rb index c78048e6a96a35..e613674af3a0f2 100644 --- a/spec/lib/gitlab/diff/highlight_spec.rb +++ b/spec/lib/gitlab/diff/highlight_spec.rb @@ -55,6 +55,18 @@ expect(subject[5].rich_text).to eq(code) end + context 'when introduce_marker_ranges is false' do + before do + stub_feature_flags(introduce_marker_ranges: false) + end + + it 'keeps the old bevavior (without mode classes)' 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 + context 'when no diff_refs' do before do allow(diff_file).to receive(:diff_refs).and_return(nil) diff --git a/spec/lib/gitlab/marker_range_spec.rb b/spec/lib/gitlab/marker_range_spec.rb index 4935e01370f4e2..5f73d2a5048ae0 100644 --- a/spec/lib/gitlab/marker_range_spec.rb +++ b/spec/lib/gitlab/marker_range_spec.rb @@ -27,6 +27,20 @@ end end + describe '#to_range' do + subject { marker_range.to_range } + + it { is_expected.to eq(first..last) } + + context 'when mode is provided' do + let(:mode) { :deletion } + + it 'is omitted during transformation' do + is_expected.not_to respond_to(:mode) + end + end + end + describe '.from_range' do subject { described_class.from_range(range) } -- GitLab From bad4d4f482d985ad9dd8f623f5195b873cdf58dc Mon Sep 17 00:00:00 2001 From: Vasilii Iakliushin Date: Thu, 11 Mar 2021 10:59:02 +0100 Subject: [PATCH 3/3] Use different cache key when feature flag is enabled --- lib/gitlab/diff/highlight_cache.rb | 7 ++++++- spec/lib/gitlab/diff/highlight_cache_spec.rb | 12 +++++++++++- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb index e0f1f8a1e58928..c5e9bfdc321baa 100644 --- a/lib/gitlab/diff/highlight_cache.rb +++ b/lib/gitlab/diff/highlight_cache.rb @@ -69,7 +69,12 @@ def clear def key strong_memoize(:redis_key) do - ['highlighted-diff-files', diffable.cache_key, VERSION, diff_options].join(":") + [ + 'highlighted-diff-files', + diffable.cache_key, VERSION, + diff_options, + Feature.enabled?(:introduce_marker_ranges, diffable.project, default_enabled: :yaml) + ].join(":") end end diff --git a/spec/lib/gitlab/diff/highlight_cache_spec.rb b/spec/lib/gitlab/diff/highlight_cache_spec.rb index efe8535ba3c861..d26bc5fc9a856f 100644 --- a/spec/lib/gitlab/diff/highlight_cache_spec.rb +++ b/spec/lib/gitlab/diff/highlight_cache_spec.rb @@ -238,7 +238,17 @@ subject { cache.key } it 'returns cache key' do - is_expected.to start_with("highlighted-diff-files:#{cache.diffable.cache_key}:2") + is_expected.to eq("highlighted-diff-files:#{cache.diffable.cache_key}:2:#{cache.diff_options}:true") + end + + context 'when 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") + end end end end -- GitLab