Merge branch '16950_improve_highlighting_for_diffs' into 'master'

Improve highlighting for merge diffs

See merge request gitlab-org/gitlab!52499

changelogs/unreleased/16950_improve_highlighting_for_diffs.yml

+---
+title: Improve highlighting for merge diffs
+merge_request: 52499
+author:
+type: added

config/feature_flags/development/improved_merge_diff_highlighting.yml

+---
+name: improved_merge_diff_highlighting
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/52499
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/299884
+milestone: '13.9'
+type: development
+group: group::source code
+default_enabled: false

ee/lib/gitlab/diff/char_diff.rb → lib/gitlab/diff/char_diff.rb

@@ -18,6 +18,33 @@ module Gitlab
         @changes
       end
 
+      def changed_ranges(offset: 0)
+        old_diffs = []
+        new_diffs = []
+        new_pointer = old_pointer = offset
+
+        generate_diff.each do |(action, content)|
+          content_size = content.size
+
+          if action == :equal
+            new_pointer += content_size
+            old_pointer += content_size
+          end
+
+          if action == :delete
+            old_diffs << (old_pointer..(old_pointer + content_size - 1))
+            old_pointer += content_size
+          end
+
+          if action == :insert
+            new_diffs << (new_pointer..(new_pointer + content_size - 1))
+            new_pointer += content_size
+          end
+        end
+
+        [old_diffs, new_diffs]
+      end
+
       def to_html
         @changes.map do |op, text|
           %{<span class="#{html_class_names(op)}">#{ERB::Util.html_escape(text)}</span>}

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
@@ -66,7 +67,7 @@ module Gitlab
       end
 
       def inline_diffs
-        @inline_diffs ||= InlineDiff.for_lines(@raw_lines)
+        @inline_diffs ||= InlineDiff.for_lines(@raw_lines, project: project)
       end
 
       def old_lines

lib/gitlab/diff/highlight_cache.rb

@@ -8,6 +8,7 @@ module Gitlab
 
       EXPIRATION = 1.week
       VERSION = 1
+      NEXT_VERSION = 2
 
       delegate :diffable,     to: :@diff_collection
       delegate :diff_options, to: :@diff_collection
@@ -69,12 +70,20 @@ 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].join(":")
         end
       end
 
       private
 
+      def version
+        if Feature.enabled?(:improved_merge_diff_highlighting, diffable.project)
+          NEXT_VERSION
+        else
+          VERSION
+        end
+      end
+
       def set_highlighted_diff_lines(diff_file, content)
         diff_file.highlighted_diff_lines = content.map do |line|
           Gitlab::Diff::Line.safe_init_from_hash(line)

lib/gitlab/diff/inline_diff.rb

@@ -27,28 +27,19 @@ module Gitlab
         @offset = offset
       end
 
-      def inline_diffs
+      def inline_diffs(project: nil)
         # Skip inline diff if empty line was replaced with content
         return if old_line == ""
 
-        lcp = longest_common_prefix(old_line, new_line)
-        lcs = longest_common_suffix(old_line[lcp..-1], new_line[lcp..-1])
-
-        lcp += offset
-        old_length = old_line.length + offset
-        new_length = new_line.length + offset
-
-        old_diff_range = lcp..(old_length - lcs - 1)
-        new_diff_range = lcp..(new_length - lcs - 1)
-
-        old_diffs = [old_diff_range] if old_diff_range.begin <= old_diff_range.end
-        new_diffs = [new_diff_range] if new_diff_range.begin <= new_diff_range.end
-
-        [old_diffs, new_diffs]
+        if Feature.enabled?(:improved_merge_diff_highlighting, project)
+          CharDiff.new(old_line, new_line).changed_ranges(offset: offset)
+        else
+          deprecated_diff
+        end
       end
 
       class << self
-        def for_lines(lines)
+        def for_lines(lines, project: nil)
           changed_line_pairs = find_changed_line_pairs(lines)
 
           inline_diffs = []
@@ -57,7 +48,7 @@ module Gitlab
             old_line = lines[old_index]
             new_line = lines[new_index]
 
-            old_diffs, new_diffs = new(old_line, new_line, offset: 1).inline_diffs
+            old_diffs, new_diffs = new(old_line, new_line, offset: 1).inline_diffs(project: project)
 
             inline_diffs[old_index] = old_diffs
             inline_diffs[new_index] = new_diffs
@@ -97,6 +88,24 @@ module Gitlab
 
       private
 
+      # See: https://gitlab.com/gitlab-org/gitlab/-/issues/299884
+      def deprecated_diff
+        lcp = longest_common_prefix(old_line, new_line)
+        lcs = longest_common_suffix(old_line[lcp..-1], new_line[lcp..-1])
+
+        lcp += offset
+        old_length = old_line.length + offset
+        new_length = new_line.length + offset
+
+        old_diff_range = lcp..(old_length - lcs - 1)
+        new_diff_range = lcp..(new_length - lcs - 1)
+
+        old_diffs = [old_diff_range] if old_diff_range.begin <= old_diff_range.end
+        new_diffs = [new_diff_range] if new_diff_range.begin <= new_diff_range.end
+
+        [old_diffs, new_diffs]
+      end
+
       def longest_common_prefix(a, b) # rubocop:disable Naming/UncommunicativeMethodParamName
         max_length = [a.length, b.length].max
 

spec/helpers/diff_helper_spec.rb

@@ -169,9 +169,9 @@ RSpec.describe DiffHelper do
     it "returns strings with marked inline diffs" do
       marked_old_line, marked_new_line = mark_inline_diffs(old_line, new_line)
 
-      expect(marked_old_line).to eq(%q{abc <span class="idiff left right deletion">&#39;def&#39;</span>})
+      expect(marked_old_line).to eq(%q{abc <span class="idiff left deletion">&#39;</span>def<span class="idiff right deletion">&#39;</span>})
       expect(marked_old_line).to be_html_safe
-      expect(marked_new_line).to eq(%q{abc <span class="idiff left right addition">&quot;def&quot;</span>})
+      expect(marked_new_line).to eq(%q{abc <span class="idiff left addition">&quot;</span>def<span class="idiff right addition">&quot;</span>})
       expect(marked_new_line).to be_html_safe
     end
 

ee/spec/lib/gitlab/diff/char_diff_spec.rb → spec/lib/gitlab/diff/char_diff_spec.rb

@@ -7,7 +7,7 @@ RSpec.describe Gitlab::Diff::CharDiff do
   let(:old_string) { "Helo \n Worlld" }
   let(:new_string) { "Hello \n World" }
 
-  subject { described_class.new(old_string, new_string) }
+  subject(:diff) { described_class.new(old_string, new_string) }
 
   describe '#generate_diff' do
     context 'when old string is nil' do
@@ -39,6 +39,28 @@ RSpec.describe Gitlab::Diff::CharDiff do
     end
   end
 
+  describe '#changed_ranges' do
+    subject { diff.changed_ranges }
+
+    context 'when old string is nil' do
+      let(:old_string) { nil }
+
+      it 'returns lists of changes' do
+        old_diffs, new_diffs = subject
+
+        expect(old_diffs).to eq([])
+        expect(new_diffs).to eq([0..12])
+      end
+    end
+
+    it 'returns ranges of changes' do
+      old_diffs, new_diffs = subject
+
+      expect(old_diffs).to eq([11..11])
+      expect(new_diffs).to eq([3..3])
+    end
+  end
+
   describe '#to_html' do
     it 'returns an HTML representation of the diff' do
       subject.generate_diff

spec/lib/gitlab/diff/highlight_cache_spec.rb

@@ -233,4 +233,22 @@ RSpec.describe Gitlab::Diff::HighlightCache, :clean_gitlab_redis_cache do
       cache.write_if_empty
     end
   end
+
+  describe '#key' do
+    subject { cache.key }
+
+    it 'returns the next version of the cache' do
+      is_expected.to start_with("highlighted-diff-files:#{cache.diffable.cache_key}:2")
+    end
+
+    context 'when feature flag is disabled' do
+      before do
+        stub_feature_flags(improved_merge_diff_highlighting: false)
+      end
+
+      it 'returns the original version of the cache' do
+        is_expected.to start_with("highlighted-diff-files:#{cache.diffable.cache_key}:1")
+      end
+    end
+  end
 end

spec/lib/gitlab/diff/inline_diff_spec.rb

@@ -37,6 +37,33 @@ RSpec.describe Gitlab::Diff::InlineDiff do
     it 'can handle unchanged empty lines' do
       expect { described_class.for_lines(['- bar', '+ baz', '']) }.not_to raise_error
     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])
+      end
+
+      context 'when feature flag is disabled' do
+        before do
+          stub_feature_flags(improved_merge_diff_highlighting: false)
+        end
+
+        it 'finds all inline diffs' do
+          expect(subject[0]).to eq([3..19])
+          expect(subject[1]).to eq([3..22])
+        end
+      end
+    end
   end
 
   describe "#inline_diffs" do