Merge branch '16950_use_marker_ranges_2' into 'master'

Store MarkerRanges in Gitlab::Diff::Line objects [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!56361

config/feature_flags/development/use_marker_ranges.yml

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

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 @@ module Gitlab
       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 @@ module Gitlab
 
       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 @@ module Gitlab
         end
       end
 
+      # Deprecated: https://gitlab.com/gitlab-org/gitlab/-/issues/324638
       def inline_diffs
         @inline_diffs ||= InlineDiff.for_lines(@raw_lines)
       end

lib/gitlab/diff/highlight_cache.rb

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

lib/gitlab/diff/inline_diff.rb

@@ -18,6 +18,7 @@ module Gitlab
         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)

lib/gitlab/diff/line.rb

@@ -8,7 +8,7 @@ module Gitlab
       #
       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 @@ module Gitlab
         # 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 @@ module Gitlab
         hash
       end
 
+      def set_marker_ranges(marker_ranges)
+        @marker_ranges = marker_ranges
+      end
+
       def old_line
         old_pos unless added? || meta?
       end

spec/lib/gitlab/diff/highlight_cache_spec.rb

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

spec/lib/gitlab/diff/highlight_spec.rb

@@ -65,6 +65,14 @@ RSpec.describe Gitlab::Diff::Highlight do
 
           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{+<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
       end
 
       context 'when no diff_refs' do
@@ -132,6 +140,18 @@ RSpec.describe Gitlab::Diff::Highlight do
         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

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

spec/lib/gitlab/diff/line_spec.rb

@@ -17,6 +17,8 @@ RSpec.describe Gitlab::Diff::Line do
                         rich_text: rich_text)
   end
 
+  let(:rich_text) { nil }
+
   describe '.init_from_hash' do
     let(:rich_text) { '<input>' }
 
@@ -51,4 +53,14 @@ RSpec.describe Gitlab::Diff::Line do
       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