Commit f55962c6 authored by Igor Drozdov's avatar Igor Drozdov

Rework implementation to using existing functionality

We're already parsing conflicts, why not using those class
parent cab493f6
......@@ -34,6 +34,7 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
environment: environment,
merge_request: @merge_request,
diff_view: diff_view,
merge_ref_head_diff: render_merge_ref_head_diff?,
pagination_data: diffs.pagination_data
}
......@@ -67,7 +68,10 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
render: ->(partial, locals) { view_to_html_string(partial, locals) }
}
options = additional_attributes.merge(diff_view: Feature.enabled?(:unified_diff_lines, @merge_request.project, default_enabled: true) ? "inline" : diff_view)
options = additional_attributes.merge(
diff_view: Feature.enabled?(:unified_diff_lines, @merge_request.project, default_enabled: true) ? "inline" : diff_view,
merge_ref_head_diff: render_merge_ref_head_diff?
)
if @merge_request.project.context_commits_enabled?
options[:context_commits] = @merge_request.recent_context_commits
......@@ -116,7 +120,7 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
end
end
if Gitlab::Utils.to_boolean(params[:diff_head]) && @merge_request.diffable_merge_ref?
if render_merge_ref_head_diff?
return CompareService.new(@project, @merge_request.merge_ref_head.sha)
.execute(@project, @merge_request.target_branch)
end
......@@ -158,6 +162,10 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
@notes = prepare_notes_for_rendering(@grouped_diff_discussions.values.flatten.flat_map(&:notes), @merge_request)
end
def render_merge_ref_head_diff?
Gitlab::Utils.to_boolean(params[:diff_head]) && @merge_request.diffable_merge_ref?
end
def note_positions
@note_positions ||= Gitlab::Diff::PositionCollection.new(renderable_notes.map(&:position))
end
......
......@@ -250,4 +250,18 @@ module DiffHelper
"...#{path[-(max - 3)..-1]}"
end
def code_navigation_path(diffs)
Gitlab::CodeNavigationPath.new(merge_request.project, diffs.diff_refs&.head_sha)
end
def conflicts
return unless options[:merge_ref_head_diff]
conflicts_service = MergeRequests::Conflicts::ListService.new(merge_request) # rubocop:disable CodeReuse/ServiceClass
return unless conflicts_service.can_be_resolved_in_ui?
conflicts_service.conflicts.files.index_by(&:our_path)
end
end
......@@ -1710,10 +1710,6 @@ class MergeRequest < ApplicationRecord
false
end
def highlight_diff_conflicts?
!branch_missing? && Feature.enabled?(:highlight_merge_conflicts_in_diff, project)
end
private
def with_rebase_lock
......
......@@ -3,6 +3,7 @@
class DiffFileEntity < DiffFileBaseEntity
include CommitsHelper
include IconsHelper
include Gitlab::Utils::StrongMemoize
expose :added_lines
expose :removed_lines
......@@ -54,11 +55,16 @@ class DiffFileEntity < DiffFileBaseEntity
# Used for inline diffs
expose :highlighted_diff_lines, using: DiffLineEntity, if: -> (diff_file, options) { inline_diff_view?(options, diff_file) && diff_file.text? } do |diff_file|
diff_file.diff_lines_for_serializer(conflicts: options[:conflicts])
file = conflict_file(options, diff_file) || diff_file
file.diff_lines_for_serializer
end
expose :is_fully_expanded do |diff_file|
diff_file.fully_expanded?
if conflict_file(options, diff_file)
false
else
diff_file.fully_expanded?
end
end
# Used for parallel diffs
......@@ -79,4 +85,10 @@ class DiffFileEntity < DiffFileBaseEntity
# If nothing is present, inline will be the default.
options.fetch(:diff_view, :inline).to_sym == :inline
end
def conflict_file(options, diff_file)
strong_memoize(:conflict_file) do
options[:conflicts] && options[:conflicts][diff_file.new_path]
end
end
end
......@@ -88,10 +88,6 @@ class DiffsEntity < Grape::Entity
private
def code_navigation_path(diffs)
Gitlab::CodeNavigationPath.new(merge_request.project, diffs.diff_refs&.head_sha)
end
def commit_ids
@commit_ids ||= merge_request.recent_commits.map(&:id)
end
......@@ -116,10 +112,4 @@ class DiffsEntity < Grape::Entity
next_commit_id: next_commit_id
)
end
def conflicts
return unless merge_request&.highlight_diff_conflicts?
MergeRequests::Conflicts::ListService.new(merge_request).conflicts # rubocop:disable CodeReuse/ServiceClass
end
end
......@@ -7,6 +7,7 @@
#
class PaginatedDiffEntity < Grape::Entity
include RequestAwareEntity
include DiffHelper
expose :diff_files do |diffs, options|
submodule_links = Gitlab::SubmoduleLinks.new(merge_request.project.repository)
......@@ -42,10 +43,6 @@ class PaginatedDiffEntity < Grape::Entity
private
def code_navigation_path(diffs)
Gitlab::CodeNavigationPath.new(merge_request.project, diffs.diff_refs&.head_sha)
end
%i[current_page next_page total_pages].each do |method|
define_method method do
pagination_data[method]
......@@ -59,10 +56,4 @@ class PaginatedDiffEntity < Grape::Entity
def merge_request
options[:merge_request]
end
def conflicts
return unless merge_request&.highlight_diff_conflicts?
MergeRequests::Conflicts::ListService.new(merge_request).conflicts # rubocop:disable CodeReuse/ServiceClass
end
end
---
name: display_merge_conflicts_in_diff
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/45008
rollout_issue_url:
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/277097
milestone: '13.5'
type: development
group: group::source code
......
---
name: highlight_merge_conflicts_in_diff
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/47003
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/277097
milestone: '13.6'
type: development
group: group::code review
default_enabled: false
......@@ -9,6 +9,11 @@ module Gitlab
CONTEXT_LINES = 3
CONFLICT_TYPES = {
"old" => "conflict_marker_their",
"new" => "conflict_marker_our"
}.freeze
attr_reader :merge_request
# 'raw' holds the Gitlab::Git::Conflict::File that this instance wraps
......@@ -46,6 +51,34 @@ module Gitlab
end
end
def diff_lines_for_serializer
# calculate sections and highlight lines before changing types
sections && highlight_lines!
sections.flat_map do |section|
if section[:conflict]
lines = []
initial_type = nil
section[:lines].each do |line|
if line.type != initial_type
lines << create_separator_line(line)
initial_type = line.type
end
line.type = CONFLICT_TYPES[line.type]
lines << line
end
lines << create_separator_line(lines.last)
lines
else
section[:lines]
end
end
end
def sections
return @sections if @sections
......@@ -93,9 +126,15 @@ module Gitlab
lines = tail_lines
elsif conflict_before
# We're at the end of the file (no conflicts after), so just remove extra
# trailing lines.
# We're at the end of the file (no conflicts after)
number_of_trailing_lines = lines.size
# Remove extra trailing lines
lines = lines.first(CONTEXT_LINES)
if number_of_trailing_lines > CONTEXT_LINES
lines << create_match_line(lines.last)
end
end
end
......@@ -117,6 +156,10 @@ module Gitlab
Gitlab::Diff::Line.new('', 'match', line.index, line.old_pos, line.new_pos)
end
def create_separator_line(line)
Gitlab::Diff::Line.new('', 'conflict_marker', line.index, nil, nil)
end
# Any line beginning with a letter, an underscore, or a dollar can be used in a
# match line header. Only context sections can contain match lines, as match lines
# have to exist in both versions of the file.
......
......@@ -195,9 +195,9 @@ module Gitlab
@highlighted_diff_lines.present?
end
def highlighted_diff_lines(conflicts: nil)
def highlighted_diff_lines
@highlighted_diff_lines ||=
Gitlab::Diff::Highlight.new(self, repository: self.repository, conflicts: conflicts).highlight
Gitlab::Diff::Highlight.new(self, repository: self.repository).highlight
end
# Array[<Hash>] with right/left keys that contains Gitlab::Diff::Line objects which text is hightlighted
......@@ -336,9 +336,9 @@ module Gitlab
# This adds the bottom match line to the array if needed. It contains
# the data to load more context lines.
def diff_lines_for_serializer(conflicts: nil)
def diff_lines_for_serializer
strong_memoize(:diff_lines_for_serializer) do
lines = highlighted_diff_lines(conflicts: conflicts)
lines = highlighted_diff_lines
next if lines.empty?
next if blob.nil?
......
......@@ -3,15 +3,12 @@
module Gitlab
module Diff
class Highlight
include Gitlab::Utils::StrongMemoize
attr_reader :diff_file, :diff_lines, :raw_lines, :repository, :conflicts
attr_reader :diff_file, :diff_lines, :raw_lines, :repository
delegate :old_path, :new_path, :old_sha, :new_sha, to: :diff_file, prefix: :diff
def initialize(diff_lines, repository: nil, conflicts: nil)
def initialize(diff_lines, repository: nil)
@repository = repository
@conflicts = conflicts
if diff_lines.is_a?(Gitlab::Diff::File)
@diff_file = diff_lines
......@@ -44,23 +41,12 @@ module Gitlab
diff_line.rich_text = rich_line
diff_line_type = conflicts_parser&.diff_line_type(diff_line.text)
diff_line.type = diff_line_type if diff_line_type
diff_line
end
end
private
def conflicts_parser
strong_memoize(:conflicts_parser) do
next unless conflicts && diff_file
Gitlab::Git::Conflict::LineParser.new(diff_file.new_path, conflicts)
end
end
def highlight_line(diff_line)
return unless diff_file && diff_file.diff_refs
......
# frozen_string_literal: true
module Gitlab
module Git
module Conflict
class LineParser
CONFLICT_OUR = 'conflict_our'
CONFLICT_THEIR = 'conflict_their'
CONFLICT_MARKER = 'conflict_marker'
attr_reader :markers
attr_accessor :type
def initialize(path, conflicts)
@markers = build_markers(path, conflicts)
@type = nil
end
def diff_line_type(line)
return unless markers
if markers.has_key?(line)
self.type = markers[line]
return CONFLICT_MARKER
end
type
end
private
def build_markers(path, conflicts)
return unless path
conflict = conflicts.files.find { |conflict| conflict.our_path == path }
return unless conflict
{
"+<<<<<<< #{conflict.our_path}" => CONFLICT_OUR,
"+=======" => CONFLICT_THEIR,
"+>>>>>>> #{conflict.their_path}" => nil
}
end
end
end
end
end
......@@ -383,6 +383,7 @@ RSpec.describe Projects::MergeRequests::DiffsController do
environment: nil,
merge_request: merge_request,
diff_view: :inline,
merge_ref_head_diff: nil,
pagination_data: {
current_page: nil,
next_page: nil,
......
......@@ -93,6 +93,51 @@ RSpec.describe Gitlab::Conflict::File do
end
end
describe '#diff_lines_for_serializer' do
let(:diff_line_types) { conflict_file.diff_lines_for_serializer.map(&:type) }
it 'assigns conflict types to the diff lines' do
expect(diff_line_types[4]).to eq('conflict_marker')
expect(diff_line_types[5..10]).to eq(['conflict_marker_our'] * 6)
expect(diff_line_types[11]).to eq('conflict_marker')
expect(diff_line_types[12..17]).to eq(['conflict_marker_their'] * 6)
expect(diff_line_types[18]).to eq('conflict_marker')
expect(diff_line_types[19..24]).to eq([nil] * 6)
expect(diff_line_types[25]).to eq('conflict_marker')
expect(diff_line_types[26..27]).to eq(['conflict_marker_our'] * 2)
expect(diff_line_types[28]).to eq('conflict_marker')
expect(diff_line_types[29..30]).to eq(['conflict_marker_their'] * 2)
expect(diff_line_types[31]).to eq('conflict_marker')
end
it 'does not add a match line to the end of the section' do
expect(diff_line_types.last).to eq(nil)
end
context 'when there are unchanged trailing lines' do
let(:rugged_conflict) { index.conflicts.first }
let(:raw_conflict_content) { index.merge_file('files/ruby/popen.rb')[:data] }
it 'assign conflict types and adds match line to the end of the section' do
expect(diff_line_types).to eq([
'match',
nil, nil, nil,
"conflict_marker",
"conflict_marker_our",
"conflict_marker",
"conflict_marker_their",
"conflict_marker_their",
"conflict_marker_their",
"conflict_marker",
nil, nil, nil,
"match"
])
end
end
end
describe '#sections' do
it 'only inserts match lines when there is a gap between sections' do
conflict_file.sections.each_with_index do |section, i|
......
......@@ -60,14 +60,12 @@ RSpec.describe Gitlab::Diff::File do
describe '#highlighted_diff_lines' do
it 'highlights the diff and memoises the result' do
conflicts = double(files: [])
expect(Gitlab::Diff::Highlight).to receive(:new)
.with(diff_file, repository: project.repository, conflicts: conflicts)
.with(diff_file, repository: project.repository)
.once
.and_call_original
diff_file.highlighted_diff_lines(conflicts: conflicts)
diff_file.highlighted_diff_lines
end
end
......
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe Gitlab::Git::Conflict::LineParser do
describe '.assign_type' do
let(:parser) { described_class.new(path, double(files: conflicts)) }
let(:path) { 'files/ruby/regex.rb' }
let(:text) do
<<~CONFLICT
module Gitlab
+<<<<<<< #{path}
def project_name_regexp
+=======
def project_name_regex
+>>>>>>> #{path}
end
CONFLICT
end
let(:diff_line_types) do
text.lines.map { |line| parser.diff_line_type(line.chomp) }
end
context 'when the file has valid conflicts' do
let(:conflicts) { [double(our_path: path, their_path: path)] }
it 'assigns conflict types to the diff lines' do
expect(diff_line_types).to eq([
nil,
'conflict_marker',
'conflict_our',
'conflict_marker',
'conflict_their',
'conflict_marker',
nil
])
end
end
context 'when the file does not have conflicts' do
let(:conflicts) { [] }
it 'does not change type of the diff lines' do
expect(diff_line_types).to eq(Array.new(7))
end
end
end
end
......@@ -4357,30 +4357,4 @@ RSpec.describe MergeRequest, factory_default: :keep do
.from(nil).to(ref)
end
end
describe '#highlight_diff_conflicts?' do
let(:merge_request) { build_stubbed(:merge_request) }
it 'returns true' do
expect(merge_request.highlight_diff_conflicts?).to eq(true)
end
context 'when branch is missing' do
let(:merge_request) { build_stubbed(:merge_request, source_branch: nil) }
it 'returns false' do
expect(merge_request.highlight_diff_conflicts?).to eq(false)
end
end
context 'highlight_merge_conflicts_in_diff feature flag is disabled' do
before do
stub_feature_flags(highlight_merge_conflicts_in_diff: false)
end
it 'returns false' do
expect(merge_request.highlight_diff_conflicts?).to eq(false)
end
end
end
end
......@@ -69,4 +69,15 @@ RSpec.describe DiffFileEntity do
end
end
end
describe '#is_fully_expanded' do
context 'file with a conflict' do
let(:options) { { conflicts: { diff_file.new_path => double(diff_lines_for_serializer: []) } } }
it 'returns false' do
expect(diff_file).not_to receive(:fully_expanded?)
expect(subject[:is_fully_expanded]).to eq(false)
end
end
end
end
......@@ -8,9 +8,12 @@ RSpec.describe DiffsEntity do
let(:request) { EntityRequest.new(project: project, current_user: user) }
let(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project) }
let(:merge_request_diffs) { merge_request.merge_request_diffs }
let(:options) do
{ request: request, merge_request: merge_request, merge_request_diffs: merge_request_diffs }
end
let(:entity) do
described_class.new(merge_request_diffs.first.diffs, request: request, merge_request: merge_request, merge_request_diffs: merge_request_diffs)
described_class.new(merge_request_diffs.first.diffs, options)
end
context 'as json' do
......@@ -70,29 +73,44 @@ RSpec.describe DiffsEntity do
end
context 'when there are conflicts' do
let(:conflicts) { double(files: []) }
let(:diff_files) { merge_request_diffs.first.diffs.diff_files }
let(:diff_file_with_conflict) { diff_files.to_a.last }
let(:diff_file_without_conflict) { diff_files.to_a[-2] }
let(:resolvable_conflicts) { true }
let(:conflict_file) { double(our_path: diff_file_with_conflict.new_path) }
let(:conflicts) { double(conflicts: double(files: [conflict_file]), can_be_resolved_in_ui?: resolvable_conflicts) }
let(:merge_ref_head_diff) { true }
let(:options) { super().merge(merge_ref_head_diff: merge_ref_head_diff) }
before do
allow_next_instance_of(MergeRequests::Conflicts::ListService) do |instance|
allow(instance).to receive(:conflicts).and_return(conflicts)
end
allow(MergeRequests::Conflicts::ListService).to receive(:new).and_return(conflicts)
end
it 'lines are parsed with passed conflicts' do
expect(Gitlab::Git::Conflict::LineParser).to(
receive(:new).exactly(17).times.with(anything, conflicts).and_call_original
)
it 'conflicts are highlighted' do
expect(conflict_file).to receive(:diff_lines_for_serializer)
expect(diff_file_with_conflict).not_to receive(:diff_lines_for_serializer)
expect(diff_file_without_conflict).to receive(:diff_lines_for_serializer).twice # for highlighted_diff_lines and is_fully_expanded
subject
end
context 'when diff lines should not be highlighted' do
before do
allow(merge_request).to receive(:highlight_diff_conflicts?).and_return(false)
context 'merge ref head diff is not chosen to be displayed' do
let(:merge_ref_head_diff) { false }
it 'conflicts are not calculated' do
expect(MergeRequests::Conflicts::ListService).not_to receive(:new)
end
end
context 'when conflicts cannot be resolved' do
let(:resolvable_conflicts) { false }
it 'conflicts has no impact on line parsing' do
expect(Gitlab::Git::Conflict::LineParser).not_to receive(:new)
it 'conflicts are not highlighted' do
expect(conflict_file).not_to receive(:diff_lines_for_serializer)
expect(diff_file_with_conflict).to receive(:diff_lines_for_serializer).twice # for highlighted_diff_lines and is_fully_expanded
expect(diff_file_without_conflict).to receive(:diff_lines_for_serializer).twice # for highlighted_diff_lines and is_fully_expanded
subject
end
......
......@@ -33,29 +33,45 @@ RSpec.describe PaginatedDiffEntity do
end
context 'when there are conflicts' do
let(:conflicts) { double(files: []) }
let(:diff_batch) { merge_request.merge_request_diff.diffs_in_batch(7, 3, diff_options: nil) }
let(:diff_files) { diff_batch.diff_files.to_a }
let(:diff_file_with_conflict) { diff_files.last }
let(:diff_file_without_conflict) { diff_files.first }
let(:resolvable_conflicts) { true }
let(:conflict_file) { double(our_path: diff_file_with_conflict.new_path) }
let(:conflicts) { double(conflicts: double(files: [conflict_file]), can_be_resolved_in_ui?: resolvable_conflicts) }
let(:merge_ref_head_diff) { true }
let(:options) { super().merge(merge_ref_head_diff: merge_ref_head_diff) }
before do
allow_next_instance_of(MergeRequests::Conflicts::ListService) do |instance|
allow(instance).to receive(:conflicts).and_return(conflicts)
end
allow(MergeRequests::Conflicts::ListService).to receive(:new).and_return(conflicts)
end
it 'lines are parsed with passed conflicts' do
expect(Gitlab::Git::Conflict::LineParser).to(
receive(:new).exactly(3).times.with(anything, conflicts).and_call_original
)
it 'conflicts are highlighted' do
expect(conflict_file).to receive(:diff_lines_for_serializer)
expect(diff_file_with_conflict).not_to receive(:diff_lines_for_serializer)
expect(diff_file_without_conflict).to receive(:diff_lines_for_serializer).twice # for highlighted_diff_lines and is_fully_expanded
subject
end
context 'when diff lines should not be highlighted' do
before do
allow(merge_request).to receive(:highlight_diff_conflicts?).and_return(false)
context 'merge ref head diff is not chosen to be displayed' do
let(:merge_ref_head_diff) { false }
it 'conflicts are not calculated' do
expect(MergeRequests::Conflicts::ListService).not_to receive(:new)
end
end
context 'when conflicts cannot be resolved' do
let(:resolvable_conflicts) { false }
it 'conflicts has no impact on line parsing' do
expect(Gitlab::Git::Conflict::LineParser).not_to receive(:new)
it 'conflicts are not highlighted' do
expect(conflict_file).not_to receive(:diff_lines_for_serializer)
expect(diff_file_with_conflict).to receive(:diff_lines_for_serializer).twice # for highlighted_diff_lines and is_fully_expanded
expect(diff_file_without_conflict).to receive(:diff_lines_for_serializer).twice # for highlighted_diff_lines and is_fully_expanded
subject
end
......
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