Commit 8dcd64a2 authored by Vasilii Iakliushin's avatar Vasilii Iakliushin

Improve highlighting for merge diffs

Contributes to https://gitlab.com/gitlab-org/gitlab/-/issues/16950

**Problem**

Current diff detection implementation has a limitation for cases with
multiple changes in the same line. It highlights the whole block
instead of marking the exact difference.

**Solution**

Use a different diff detection algorithm to improve the detection

Other changes:

Extract char_diff.rb from ee namespace
parent b9a474ad
...@@ -18,6 +18,33 @@ module Gitlab ...@@ -18,6 +18,33 @@ module Gitlab
@changes @changes
end 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 def to_html
@changes.map do |op, text| @changes.map do |op, text|
%{<span class="#{html_class_names(op)}">#{ERB::Util.html_escape(text)}</span>} %{<span class="#{html_class_names(op)}">#{ERB::Util.html_escape(text)}</span>}
......
...@@ -31,20 +31,7 @@ module Gitlab ...@@ -31,20 +31,7 @@ module Gitlab
# Skip inline diff if empty line was replaced with content # Skip inline diff if empty line was replaced with content
return if old_line == "" return if old_line == ""
lcp = longest_common_prefix(old_line, new_line) CharDiff.new(old_line, new_line).changed_ranges(offset: offset)
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 end
class << self class << self
......
...@@ -169,9 +169,9 @@ RSpec.describe DiffHelper do ...@@ -169,9 +169,9 @@ RSpec.describe DiffHelper do
it "returns strings with marked inline diffs" do it "returns strings with marked inline diffs" do
marked_old_line, marked_new_line = mark_inline_diffs(old_line, new_line) 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_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 expect(marked_new_line).to be_html_safe
end end
......
...@@ -7,7 +7,7 @@ RSpec.describe Gitlab::Diff::CharDiff do ...@@ -7,7 +7,7 @@ RSpec.describe Gitlab::Diff::CharDiff do
let(:old_string) { "Helo \n Worlld" } let(:old_string) { "Helo \n Worlld" }
let(:new_string) { "Hello \n World" } 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 describe '#generate_diff' do
context 'when old string is nil' do context 'when old string is nil' do
...@@ -39,6 +39,28 @@ RSpec.describe Gitlab::Diff::CharDiff do ...@@ -39,6 +39,28 @@ RSpec.describe Gitlab::Diff::CharDiff do
end end
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 describe '#to_html' do
it 'returns an HTML representation of the diff' do it 'returns an HTML representation of the diff' do
subject.generate_diff subject.generate_diff
......
...@@ -37,6 +37,22 @@ RSpec.describe Gitlab::Diff::InlineDiff do ...@@ -37,6 +37,22 @@ RSpec.describe Gitlab::Diff::InlineDiff do
it 'can handle unchanged empty lines' do it 'can handle unchanged empty lines' do
expect { described_class.for_lines(['- bar', '+ baz', '']) }.not_to raise_error expect { described_class.for_lines(['- bar', '+ baz', '']) }.not_to raise_error
end 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
end
end end
describe "#inline_diffs" do describe "#inline_diffs" do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment