From 257987aac53d376851f5d98037c51d52581082f9 Mon Sep 17 00:00:00 2001 From: Vasilii Iakliushin Date: Thu, 11 Mar 2021 18:08:28 +0100 Subject: [PATCH] Detect MarkerRanges for word-diff mode Contributes to https://gitlab.com/gitlab-org/gitlab/-/issues/16950 Follow-up for https://gitlab.com/gitlab-org/gitlab/-/merge_requests/55105 Extend to Gitlab::WordDiff::ChunkCollection class to generate `marker_ranges` based on chunks received from `git diff --word-diff=porcelain` output. Each chunk can be an unchanged string (starts with ` `), new string (starts with `+`) or removed string (starts with `-`). While parsing we collect chunks until we receive a newline delimiter. Then we are combining them together to generate Gitlab::Diff::Line object and assign MarkerRanges for changed parts of the diff line. --- lib/gitlab/marker_range.rb | 6 +++ lib/gitlab/word_diff/chunk_collection.rb | 21 ++++++++++ lib/gitlab/word_diff/parser.rb | 2 +- spec/lib/gitlab/diff/char_diff_spec.rb | 6 +-- spec/lib/gitlab/marker_range_spec.rb | 33 +++++++++++++-- .../gitlab/word_diff/chunk_collection_spec.rb | 23 +++++++++++ spec/lib/gitlab/word_diff/parser_spec.rb | 40 +++++++++++++++---- 7 files changed, 116 insertions(+), 15 deletions(-) diff --git a/lib/gitlab/marker_range.rb b/lib/gitlab/marker_range.rb index 50a59adebdf1ea..73e4a545679fbf 100644 --- a/lib/gitlab/marker_range.rb +++ b/lib/gitlab/marker_range.rb @@ -24,6 +24,12 @@ def to_range Range.new(self.begin, self.end, self.exclude_end?) end + def ==(other) + return false unless other.is_a?(self.class) + + self.mode == other.mode && super + end + attr_reader :mode end end diff --git a/lib/gitlab/word_diff/chunk_collection.rb b/lib/gitlab/word_diff/chunk_collection.rb index dd388f753029a8..d5c3e59d405149 100644 --- a/lib/gitlab/word_diff/chunk_collection.rb +++ b/lib/gitlab/word_diff/chunk_collection.rb @@ -18,6 +18,27 @@ def content def reset @chunks = [] end + + def marker_ranges + start = 0 + + @chunks.each_with_object([]) do |element, ranges| + mode = mode_for_element(element) + + ranges << Gitlab::MarkerRange.new(start, start + element.length - 1, mode: mode) if mode + + start += element.length + end + end + + private + + def mode_for_element(element) + return Gitlab::MarkerRange::DELETION if element.removed? + return Gitlab::MarkerRange::ADDITION if element.added? + + nil + end end end end diff --git a/lib/gitlab/word_diff/parser.rb b/lib/gitlab/word_diff/parser.rb index 3b6d4d4d3844cb..e611abb5692e17 100644 --- a/lib/gitlab/word_diff/parser.rb +++ b/lib/gitlab/word_diff/parser.rb @@ -31,7 +31,7 @@ def parse(lines, diff_file: nil) @chunks.add(segment) when Segments::Newline - yielder << build_line(@chunks.content, nil, parent_file: diff_file) + yielder << build_line(@chunks.content, nil, parent_file: diff_file).tap { |line| line.set_marker_ranges(@chunks.marker_ranges) } @chunks.reset counter.increase_pos_num diff --git a/spec/lib/gitlab/diff/char_diff_spec.rb b/spec/lib/gitlab/diff/char_diff_spec.rb index e4e2a3ba050b94..d38008c16f235d 100644 --- a/spec/lib/gitlab/diff/char_diff_spec.rb +++ b/spec/lib/gitlab/diff/char_diff_spec.rb @@ -49,15 +49,15 @@ old_diffs, new_diffs = subject expect(old_diffs).to eq([]) - expect(new_diffs).to eq([0..12]) + expect(new_diffs).to eq([Gitlab::MarkerRange.new(0, 12, mode: :addition)]) 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]) + expect(old_diffs).to eq([Gitlab::MarkerRange.new(11, 11, mode: :deletion)]) + expect(new_diffs).to eq([Gitlab::MarkerRange.new(3, 3, mode: :addition)]) end end diff --git a/spec/lib/gitlab/marker_range_spec.rb b/spec/lib/gitlab/marker_range_spec.rb index 5f73d2a5048ae0..c4670ec58a8b69 100644 --- a/spec/lib/gitlab/marker_range_spec.rb +++ b/spec/lib/gitlab/marker_range_spec.rb @@ -9,7 +9,7 @@ let(:last) { 10 } let(:mode) { nil } - it { is_expected.to eq(first..last) } + it { expect(marker_range.to_range).to eq(first..last) } it 'behaves like a Range' do is_expected.to be_kind_of(Range) @@ -51,14 +51,14 @@ end it 'keeps correct range' do - is_expected.to eq(range) + is_expected.to eq(described_class.new(1, 3)) end context 'when range excludes end' do let(:range) { 1...3 } it 'keeps correct range' do - is_expected.to eq(range) + is_expected.to eq(described_class.new(1, 3, exclude_end: true)) end end @@ -68,4 +68,31 @@ it { is_expected.to be(marker_range) } end end + + describe '#==' do + subject { default_marker_range == another_marker_range } + + let(:default_marker_range) { described_class.new(0, 1, mode: :addition) } + let(:another_marker_range) { default_marker_range } + + it { is_expected.to be_truthy } + + context 'when marker ranges have different modes' do + let(:another_marker_range) { described_class.new(0, 1, mode: :deletion) } + + it { is_expected.to be_falsey } + end + + context 'when marker ranges have different ranges' do + let(:another_marker_range) { described_class.new(0, 2, mode: :addition) } + + it { is_expected.to be_falsey } + end + + context 'when marker ranges is a simple range' do + let(:another_marker_range) { (0..1) } + + it { is_expected.to be_falsey } + end + end end diff --git a/spec/lib/gitlab/word_diff/chunk_collection_spec.rb b/spec/lib/gitlab/word_diff/chunk_collection_spec.rb index aa837f760c1907..73e9ff3974aa02 100644 --- a/spec/lib/gitlab/word_diff/chunk_collection_spec.rb +++ b/spec/lib/gitlab/word_diff/chunk_collection_spec.rb @@ -41,4 +41,27 @@ expect(collection.content).to eq('') end end + + describe '#marker_ranges' do + let(:chunks) do + [ + Gitlab::WordDiff::Segments::Chunk.new(' Hello '), + Gitlab::WordDiff::Segments::Chunk.new('-World'), + Gitlab::WordDiff::Segments::Chunk.new('+GitLab'), + Gitlab::WordDiff::Segments::Chunk.new('+!!!') + ] + end + + it 'returns marker ranges for every chunk with changes' do + chunks.each { |chunk| collection.add(chunk) } + + expect(collection.marker_ranges).to eq( + [ + Gitlab::MarkerRange.new(6, 10, mode: :deletion), + Gitlab::MarkerRange.new(11, 16, mode: :addition), + Gitlab::MarkerRange.new(17, 19, mode: :addition) + ] + ) + end + end end diff --git a/spec/lib/gitlab/word_diff/parser_spec.rb b/spec/lib/gitlab/word_diff/parser_spec.rb index 3aeefb57a02065..e793e44fd451d6 100644 --- a/spec/lib/gitlab/word_diff/parser_spec.rb +++ b/spec/lib/gitlab/word_diff/parser_spec.rb @@ -36,15 +36,26 @@ aggregate_failures do expect(diff_lines.count).to eq(7) - expect(diff_lines.map(&:to_hash)).to match_array( + expect(diff_lines.map { |line| diff_line_attributes(line) }).to eq( [ - a_hash_including(index: 0, old_pos: 1, new_pos: 1, text: '', type: nil), - a_hash_including(index: 1, old_pos: 2, new_pos: 2, text: 'Unchanged line', type: nil), - a_hash_including(index: 2, old_pos: 3, new_pos: 3, text: '', type: nil), - a_hash_including(index: 3, old_pos: 4, new_pos: 4, text: 'Old changeNew addition unchanged content', type: nil), - a_hash_including(index: 4, old_pos: 50, new_pos: 50, text: '@@ -50,14 +50,13 @@', type: 'match'), - a_hash_including(index: 5, old_pos: 50, new_pos: 50, text: 'First change same same same_removed_added_end of the line', type: nil), - a_hash_including(index: 6, old_pos: 51, new_pos: 51, text: '', type: nil) + { index: 0, old_pos: 1, new_pos: 1, text: '', type: nil, marker_ranges: [] }, + { index: 1, old_pos: 2, new_pos: 2, text: 'Unchanged line', type: nil, marker_ranges: [] }, + { index: 2, old_pos: 3, new_pos: 3, text: '', type: nil, marker_ranges: [] }, + { index: 3, old_pos: 4, new_pos: 4, text: 'Old changeNew addition unchanged content', type: nil, + marker_ranges: [ + Gitlab::MarkerRange.new(0, 9, mode: :deletion), + Gitlab::MarkerRange.new(10, 21, mode: :addition) + ] }, + + { index: 4, old_pos: 50, new_pos: 50, text: '@@ -50,14 +50,13 @@', type: 'match', marker_ranges: [] }, + { index: 5, old_pos: 50, new_pos: 50, text: 'First change same same same_removed_added_end of the line', type: nil, + marker_ranges: [ + Gitlab::MarkerRange.new(0, 11, mode: :addition), + Gitlab::MarkerRange.new(28, 35, mode: :deletion), + Gitlab::MarkerRange.new(36, 41, mode: :addition) + ] }, + + { index: 6, old_pos: 51, new_pos: 51, text: '', type: nil, marker_ranges: [] } ] ) end @@ -64,4 +75,17 @@ it { is_expected.to eq([]) } end end + + private + + def diff_line_attributes(diff_line) + { + index: diff_line.index, + old_pos: diff_line.old_pos, + new_pos: diff_line.new_pos, + text: diff_line.text, + type: diff_line.type, + marker_ranges: diff_line.marker_ranges + } + end end -- GitLab