Merge branch '16950_add_marker_ranges' into 'master'

Introduce MarkerRange class [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!55669

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

app/services/system_notes/issuables_service.rb

@@ -125,8 +125,8 @@ module SystemNotes
 
       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}**"
 

config/feature_flags/development/introduce_marker_ranges.yml

+---
+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

lib/banzai/filter/autolink_filter.rb

@@ -120,7 +120,7 @@ module Banzai
       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

lib/banzai/filter/spaced_link_filter.rb

@@ -76,7 +76,7 @@ module Banzai
       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

lib/gitlab/dependency_linker/base_linker.rb

@@ -80,7 +80,7 @@ module Gitlab
         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

lib/gitlab/dependency_linker/go_mod_linker.rb

@@ -22,7 +22,7 @@ module Gitlab
 
           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

lib/gitlab/dependency_linker/go_sum_linker.rb

@@ -21,7 +21,7 @@ module Gitlab
           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

lib/gitlab/diff/char_diff.rb

@@ -32,12 +32,12 @@ module Gitlab
           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

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 @@ module Gitlab
 
           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

lib/gitlab/diff/highlight_cache.rb

@@ -69,7 +69,12 @@ module Gitlab
 
       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
 

lib/gitlab/diff/inline_diff_markdown_marker.rb

@@ -8,8 +8,8 @@ module Gitlab
         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

lib/gitlab/diff/inline_diff_marker.rb

@@ -7,8 +7,8 @@ module Gitlab
         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:|
           %{<span class="#{html_class_names(left, right, mode)}">#{text}</span>}.html_safe
         end
       end

lib/gitlab/marker_range.rb

+# 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
+
+    def to_range
+      Range.new(self.begin, self.end, self.exclude_end?)
+    end
+
+    attr_reader :mode
+  end
+end

lib/gitlab/string_range_marker.rb

@@ -15,8 +15,10 @@ module Gitlab
       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 @@ module Gitlab
           # 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 @@ module Gitlab
         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 @@ module Gitlab
     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

spec/lib/gitlab/diff/highlight_cache_spec.rb

@@ -238,7 +238,17 @@ RSpec.describe Gitlab::Diff::HighlightCache, :clean_gitlab_redis_cache do
     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

spec/lib/gitlab/diff/highlight_spec.rb

@@ -50,11 +50,23 @@ RSpec.describe Gitlab::Diff::Highlight do
       end
 
       it 'highlights and marks added lines' do
-        code = %Q{+<span id="LC9" class="line" lang="ruby">      <span class="k">raise</span> <span class="no"><span class="idiff left">RuntimeError</span></span><span class="p"><span class="idiff">,</span></span><span class="idiff right"> </span><span class="s2">"System commands must be given as an array of strings"</span></span>\n}
+        code = %Q{+<span id="LC9" class="line" lang="ruby">      <span class="k">raise</span> <span class="no"><span class="idiff left addition">RuntimeError</span></span><span class="p"><span class="idiff addition">,</span></span><span class="idiff right addition"> </span><span class="s2">"System commands must be given as an array of strings"</span></span>\n}
 
         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{+<span id="LC9" class="line" lang="ruby">      <span class="k">raise</span> <span class="no"><span class="idiff left">RuntimeError</span></span><span class="p"><span class="idiff">,</span></span><span class="idiff right"> </span><span class="s2">"System commands must be given as an array of strings"</span></span>\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)
@@ -93,7 +105,7 @@ RSpec.describe Gitlab::Diff::Highlight do
       end
 
       it 'marks added lines' do
-        code = %q{+      raise <span class="idiff left right">RuntimeError, </span>&quot;System commands must be given as an array of strings&quot;}
+        code = %q{+      raise <span class="idiff left right addition">RuntimeError, </span>&quot;System commands must be given as an array of strings&quot;}
 
         expect(subject[5].rich_text).to eq(code)
         expect(subject[5].rich_text).to be_html_safe

spec/lib/gitlab/diff/inline_diff_markdown_marker_spec.rb

@@ -5,8 +5,8 @@ require 'spec_helper'
 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'")

spec/lib/gitlab/marker_range_spec.rb

+# 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 '#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) }
+
+    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

spec/lib/gitlab/string_range_marker_spec.rb

@@ -8,7 +8,7 @@ RSpec.describe Gitlab::StringRangeMarker do
       raw = 'abc <def>'
       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

spec/lib/gitlab/string_regex_marker_spec.rb

@@ -9,7 +9,7 @@ RSpec.describe Gitlab::StringRegexMarker do
       let(:rich) { %{<span class="key">"name"</span><span class="punctuation">: </span><span class="value">"AFNetworking"</span>}.html_safe }
 
       subject do
-        described_class.new(raw, rich).mark(/"[^"]+":\s*"(?<name>[^"]+)"/, group: :name) do |text, left:, right:|
+        described_class.new(raw, rich).mark(/"[^"]+":\s*"(?<name>[^"]+)"/, group: :name) do |text, left:, right:, mode:|
           %{<a href="#">#{text}</a>}.html_safe
         end
       end
@@ -25,7 +25,7 @@ RSpec.describe Gitlab::StringRegexMarker do
       let(:rich) { %{a &lt;b&gt; &lt;c&gt; 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:|
           %{<strong>#{text}</strong>}.html_safe
         end
       end