Skip to content
Snippets Groups Projects
Commit 4bd54349 authored by Patrick Bajao's avatar Patrick Bajao :one:
Browse files

Remove highlighting conflicts on diff

Since it's currently confusing to see conflicts inline on the
diff, we are removing the functionality to show conflicts on
diff.

We still keep the functionality to show the conflict type alert
(per file) on the diff though.

This is behind `display_merge_conflicts_in_diff` feature flag.
parent 9d5a0d0f
No related branches found
No related tags found
1 merge request!100786Remove highlighting conflicts on diff
Showing
with 100 additions and 257 deletions
......@@ -44,7 +44,7 @@ def diffs_batch
diff_view: diff_view,
merge_ref_head_diff: render_merge_ref_head_diff?,
pagination_data: diffs.pagination_data,
allow_tree_conflicts: display_merge_conflicts_in_diff?
merge_conflicts_in_diff: display_merge_conflicts_in_diff?
}
# NOTE: Any variables that would affect the resulting json needs to be added to the cache_context to avoid stale cache issues.
......@@ -57,7 +57,7 @@ def diffs_batch
params[:page],
params[:per_page],
options[:merge_ref_head_diff],
options[:allow_tree_conflicts]
options[:merge_conflicts_in_diff]
]
if Feature.enabled?(:check_etags_diffs_batch_before_write_cache, merge_request.project) && !stale?(etag: [cache_context + diff_options_hash.fetch(:paths, []), diffs])
......@@ -81,7 +81,7 @@ def diffs_metadata
options = additional_attributes.merge(
only_context_commits: show_only_context_commits?,
merge_ref_head_diff: render_merge_ref_head_diff?,
allow_tree_conflicts: display_merge_conflicts_in_diff?
merge_conflicts_in_diff: display_merge_conflicts_in_diff?
)
render json: DiffsMetadataSerializer.new(project: @merge_request.project, current_user: current_user)
......@@ -110,7 +110,7 @@ def render_diffs
options = additional_attributes.merge(
diff_view: "inline",
merge_ref_head_diff: render_merge_ref_head_diff?,
allow_tree_conflicts: display_merge_conflicts_in_diff?
merge_conflicts_in_diff: display_merge_conflicts_in_diff?
)
options[:context_commits] = @merge_request.recent_context_commits
......
......@@ -227,7 +227,6 @@ def render_fork_suggestion
end
def conflicts(allow_tree_conflicts: false)
return unless options[:merge_ref_head_diff]
return unless merge_request.cannot_be_merged?
conflicts_service = MergeRequests::Conflicts::ListService.new(merge_request, allow_tree_conflicts: allow_tree_conflicts) # rubocop:disable CodeReuse/ServiceClass
......
......@@ -1134,7 +1134,7 @@ def check_mergeability(async: false)
# rubocop: enable CodeReuse/ServiceClass
def diffable_merge_ref?
open? && merge_head_diff.present? && (Feature.enabled?(:display_merge_conflicts_in_diff, project) || can_be_merged?)
open? && merge_head_diff.present? && can_be_merged?
end
# Returns boolean indicating the merge_status should be rechecked in order to
......
......@@ -55,17 +55,9 @@ class DiffFileEntity < DiffFileBaseEntity
end
# Used for inline diffs
expose :highlighted_diff_lines, using: DiffLineEntity, if: -> (diff_file, options) { inline_diff_view?(options) && diff_file.text? } do |diff_file|
highlighted_diff_lines_for(diff_file, options)
end
expose :diff_lines_for_serializer, as: :highlighted_diff_lines, using: DiffLineEntity, if: -> (diff_file, options) { inline_diff_view?(options) && diff_file.text? }
expose :is_fully_expanded do |diff_file|
if conflict_file(options, diff_file)
false
else
diff_file.fully_expanded?
end
end
expose :fully_expanded?, as: :is_fully_expanded
# Used for parallel diffs
expose :parallel_diff_lines, using: DiffLineParallelEntity, if: -> (diff_file, options) { parallel_diff_view?(options) && diff_file.text? }
......@@ -88,15 +80,6 @@ def diff_view(options)
# If nothing is present, inline will be the default.
options.fetch(:diff_view, :inline).to_sym
end
def highlighted_diff_lines_for(diff_file, options)
file = conflict_file(options, diff_file) || diff_file
file.diff_lines_for_serializer
rescue Gitlab::Git::Conflict::Parser::UnmergeableFile
# Fallback to diff_file as it means that conflict lines can't be parsed due to limit
diff_file.diff_lines_for_serializer
end
end
DiffFileEntity.prepend_mod
......@@ -74,7 +74,7 @@ class DiffsEntity < Grape::Entity
options.merge(
submodule_links: submodule_links,
code_navigation_path: code_navigation_path(diffs),
conflicts: conflicts(allow_tree_conflicts: options[:allow_tree_conflicts])
conflicts: (conflicts(allow_tree_conflicts: true) if options[:merge_conflicts_in_diff])
)
)
end
......
......@@ -6,7 +6,7 @@ class DiffsMetadataEntity < DiffsEntity
DiffFileMetadataEntity.represent(
diffs.raw_diff_files(sorted: true),
options.merge(
conflicts: conflicts(allow_tree_conflicts: options[:allow_tree_conflicts])
conflicts: (conflicts(allow_tree_conflicts: true) if options[:merge_conflicts_in_diff])
)
)
end
......
......@@ -17,7 +17,7 @@ class PaginatedDiffEntity < Grape::Entity
options.merge(
submodule_links: submodule_links,
code_navigation_path: code_navigation_path(diffs),
conflicts: conflicts(allow_tree_conflicts: options[:allow_tree_conflicts])
conflicts: (conflicts(allow_tree_conflicts: true) if options[:merge_conflicts_in_diff])
)
)
end
......
......@@ -156,8 +156,7 @@ def can_git_merge?
end
def merge_to_ref
params = { allow_conflicts: Feature.enabled?(:display_merge_conflicts_in_diff, project) }
result = MergeRequests::MergeToRefService.new(project: project, current_user: merge_request.author, params: params).execute(merge_request)
result = MergeRequests::MergeToRefService.new(project: project, current_user: merge_request.author, params: {}).execute(merge_request)
result[:status] == :success
end
......
......@@ -213,7 +213,7 @@ def go(extra_params = {})
commit: nil,
latest_diff: true,
only_context_commits: false,
allow_tree_conflicts: true,
merge_conflicts_in_diff: true,
merge_ref_head_diff: false
}
end
......@@ -281,7 +281,7 @@ def go(extra_params = {})
commit: nil,
latest_diff: true,
only_context_commits: false,
allow_tree_conflicts: true,
merge_conflicts_in_diff: true,
merge_ref_head_diff: nil
}
end
......@@ -303,7 +303,7 @@ def go(extra_params = {})
commit: merge_request.diff_head_commit,
latest_diff: nil,
only_context_commits: false,
allow_tree_conflicts: true,
merge_conflicts_in_diff: true,
merge_ref_head_diff: nil
}
end
......@@ -329,7 +329,7 @@ def go(extra_params = {})
commit: nil,
latest_diff: true,
only_context_commits: false,
allow_tree_conflicts: false,
merge_conflicts_in_diff: false,
merge_ref_head_diff: nil
}
end
......@@ -488,7 +488,7 @@ def collection_arguments(pagination_data = {})
commit: nil,
diff_view: :inline,
merge_ref_head_diff: nil,
allow_tree_conflicts: true,
merge_conflicts_in_diff: true,
pagination_data: {
total_pages: nil
}.merge(pagination_data)
......@@ -616,7 +616,7 @@ def go(extra_params = {})
it_behaves_like 'serializes diffs with expected arguments' do
let(:collection) { Gitlab::Diff::FileCollection::MergeRequestDiffBatch }
let(:expected_options) { collection_arguments(total_pages: 20).merge(allow_tree_conflicts: false) }
let(:expected_options) { collection_arguments(total_pages: 20).merge(merge_conflicts_in_diff: false) }
end
it_behaves_like 'successful request'
......
......@@ -471,7 +471,6 @@
describe '#conflicts' do
let(:merge_request) { instance_double(MergeRequest, cannot_be_merged?: true) }
let(:merge_ref_head_diff) { true }
let(:can_be_resolved_in_ui?) { true }
let(:allow_tree_conflicts) { false }
let(:files) { [instance_double(Gitlab::Conflict::File, path: 'a')] }
......@@ -479,7 +478,6 @@
before do
allow(helper).to receive(:merge_request).and_return(merge_request)
allow(helper).to receive(:options).and_return(merge_ref_head_diff: merge_ref_head_diff)
allow_next_instance_of(MergeRequests::Conflicts::ListService, merge_request, allow_tree_conflicts: allow_tree_conflicts) do |svc|
allow(svc).to receive(:can_be_resolved_in_ui?).and_return(can_be_resolved_in_ui?)
......@@ -496,14 +494,6 @@
expect(helper.conflicts).to eq('a' => files.first)
end
context 'when merge_ref_head_diff option is false' do
let(:merge_ref_head_diff) { false }
it 'returns nil' do
expect(helper.conflicts).to be_nil
end
end
context 'when merge request can be merged' do
let(:merge_request) { instance_double(MergeRequest, cannot_be_merged?: false) }
......
......@@ -5138,17 +5138,7 @@ def transition!
end
it 'returns false' do
expect(merge_request.diffable_merge_ref?).to eq(true)
end
context 'display_merge_conflicts_in_diff is disabled' do
before do
stub_feature_flags(display_merge_conflicts_in_diff: false)
end
it 'returns false' do
expect(merge_request.diffable_merge_ref?).to eq(false)
end
expect(merge_request.diffable_merge_ref?).to eq(false)
end
end
end
......
......@@ -35,7 +35,7 @@ def collection_arguments(pagination_data = {})
commit: nil,
diff_view: :inline,
merge_ref_head_diff: nil,
allow_tree_conflicts: true,
merge_conflicts_in_diff: true,
pagination_data: {
total_pages: nil
}.merge(pagination_data)
......
......@@ -33,7 +33,7 @@ def collection_arguments(pagination_data = {})
commit: nil,
diff_view: :inline,
merge_ref_head_diff: nil,
allow_tree_conflicts: true,
merge_conflicts_in_diff: true,
pagination_data: {
total_pages: nil
}.merge(pagination_data)
......@@ -128,7 +128,7 @@ def go(headers: {}, **extra_params)
context 'with disabled display_merge_conflicts_in_diff feature' do
let(:collection) { Gitlab::Diff::FileCollection::MergeRequestDiffBatch }
let(:expected_options) { collection_arguments(total_pages: 20).merge(allow_tree_conflicts: false) }
let(:expected_options) { collection_arguments(total_pages: 20).merge(merge_conflicts_in_diff: false) }
before do
stub_feature_flags(display_merge_conflicts_in_diff: false)
......
......@@ -80,47 +80,13 @@
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: [], conflict_type: :both_modified) } } }
it 'returns false' do
expect(diff_file).not_to receive(:fully_expanded?)
expect(subject[:is_fully_expanded]).to eq(false)
end
end
end
describe '#highlighted_diff_lines' do
context 'file without a conflict' do
let(:options) { { conflicts: {} } }
let(:options) { { conflicts: {} } }
it 'calls diff_lines_for_serializer on diff_file' do
# #diff_lines_for_serializer gets called in #fully_expanded? as well so we expect twice
expect(diff_file).to receive(:diff_lines_for_serializer).twice.and_return([])
expect(subject[:highlighted_diff_lines]).to eq([])
end
end
context 'file with a conflict' do
let(:conflict_file) { instance_double(Gitlab::Conflict::File, conflict_type: :both_modified) }
let(:options) { { conflicts: { diff_file.new_path => conflict_file } } }
it 'calls diff_lines_for_serializer on matching conflict file' do
expect(conflict_file).to receive(:diff_lines_for_serializer).and_return([])
expect(subject[:highlighted_diff_lines]).to eq([])
end
context 'when Gitlab::Git::Conflict::Parser::UnmergeableFile gets raised' do
before do
allow(conflict_file).to receive(:diff_lines_for_serializer).and_raise(Gitlab::Git::Conflict::Parser::UnmergeableFile)
end
it 'falls back to diff_file diff_lines_for_serializer' do
expect(diff_file).to receive(:diff_lines_for_serializer).and_return([])
expect(subject[:highlighted_diff_lines]).to eq([])
end
end
it 'calls diff_lines_for_serializer on diff_file' do
# #diff_lines_for_serializer gets called in #fully_expanded? as well so we expect twice
expect(diff_file).to receive(:diff_lines_for_serializer).twice.and_return([])
expect(subject[:highlighted_diff_lines]).to eq([])
end
end
......
......@@ -9,13 +9,13 @@
let(:request) { EntityRequest.new(project: project, current_user: user) }
let(:merge_request_diffs) { merge_request.merge_request_diffs }
let(:allow_tree_conflicts) { false }
let(:merge_conflicts_in_diff) { false }
let(:options) do
{
request: request,
merge_request: merge_request,
merge_request_diffs: merge_request_diffs,
allow_tree_conflicts: allow_tree_conflicts
merge_conflicts_in_diff: merge_conflicts_in_diff
}
end
......@@ -87,60 +87,39 @@
end
end
context 'when there are conflicts' do
describe 'diff_files' do
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(path: diff_file_with_conflict.new_path, conflict_type: :both_modified) }
let(:conflicts) { double(conflicts: double(files: [conflict_file]), can_be_resolved_in_ui?: resolvable_conflicts) }
it 'serializes diff files using DiffFileEntity' do
expect(DiffFileEntity)
.to receive(:represent)
.with(
diff_files,
hash_including(options.merge(conflicts: nil))
)
let(:merge_ref_head_diff) { true }
let(:options) { super().merge(merge_ref_head_diff: merge_ref_head_diff) }
before do
allow(merge_request).to receive(:cannot_be_merged?).and_return(true)
allow(MergeRequests::Conflicts::ListService).to receive(:new).and_return(conflicts)
end
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 '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
subject[:diff_files]
end
context 'when conflicts cannot be resolved' do
let(:resolvable_conflicts) { false }
context 'when merge_conflicts_in_diff is true' do
let(:conflict_file) { double(path: diff_files.first.new_path, conflict_type: :both_modified) }
let(:conflicts) { double(conflicts: double(files: [conflict_file]), can_be_resolved_in_ui?: false) }
let(:merge_conflicts_in_diff) { true }
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
before do
allow(merge_request).to receive(:cannot_be_merged?).and_return(true)
allow(MergeRequests::Conflicts::ListService).to receive(:new).and_return(conflicts)
end
context 'when allow_tree_conflicts is set to true' do
let(:allow_tree_conflicts) { true }
it 'conflicts are still 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
it 'serializes diff files with conflicts' do
expect(DiffFileEntity)
.to receive(:represent)
.with(
diff_files,
hash_including(options.merge(conflicts: { conflict_file.path => conflict_file }))
)
subject
end
subject[:diff_files]
end
end
end
......
......@@ -9,6 +9,7 @@
let(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project) }
let(:merge_request_diffs) { merge_request.merge_request_diffs }
let(:merge_request_diff) { merge_request_diffs.last }
let(:merge_conflicts_in_diff) { false }
let(:options) { {} }
let(:entity) do
......@@ -17,7 +18,8 @@
options.merge(
request: request,
merge_request: merge_request,
merge_request_diffs: merge_request_diffs
merge_request_diffs: merge_request_diffs,
merge_conflicts_in_diff: merge_conflicts_in_diff
)
)
end
......@@ -54,49 +56,36 @@
end
end
it 'returns diff files metadata' do
payload = DiffFileMetadataEntity.represent(raw_diff_files).as_json
it 'serializes diff files metadata using DiffFileMetadataEntity' do
expect(DiffFileMetadataEntity)
.to receive(:represent)
.with(
raw_diff_files,
hash_including(options.merge(conflicts: nil))
)
expect(subject[:diff_files]).to eq(payload)
subject[:diff_files]
end
context 'when merge_ref_head_diff and allow_tree_conflicts options are set' do
context 'when merge_conflicts_in_diff is true' do
let(:conflict_file) { double(path: raw_diff_files.first.new_path, conflict_type: :both_modified) }
let(:conflicts) { double(conflicts: double(files: [conflict_file]), can_be_resolved_in_ui?: false) }
let(:merge_conflicts_in_diff) { true }
before do
allow(merge_request).to receive(:cannot_be_merged?).and_return(true)
allow(MergeRequests::Conflicts::ListService).to receive(:new).and_return(conflicts)
end
context 'when merge_ref_head_diff is true and allow_tree_conflicts is false' do
let(:options) { { merge_ref_head_diff: true, allow_tree_conflicts: false } }
it 'serializes diff files with conflicts' do
expect(DiffFileMetadataEntity)
.to receive(:represent)
.with(
raw_diff_files,
hash_including(options.merge(conflicts: { conflict_file.path => conflict_file }))
)
it 'returns diff files metadata without conflicts' do
payload = DiffFileMetadataEntity.represent(raw_diff_files).as_json
expect(subject[:diff_files]).to eq(payload)
end
end
context 'when merge_ref_head_diff is false and allow_tree_conflicts is true' do
let(:options) { { merge_ref_head_diff: false, allow_tree_conflicts: true } }
it 'returns diff files metadata without conflicts' do
payload = DiffFileMetadataEntity.represent(raw_diff_files).as_json
expect(subject[:diff_files]).to eq(payload)
end
end
context 'when merge_ref_head_diff and allow_tree_conflicts are true' do
let(:options) { { merge_ref_head_diff: true, allow_tree_conflicts: true } }
it 'returns diff files metadata with conflicts' do
payload = DiffFileMetadataEntity.represent(raw_diff_files, conflicts: { conflict_file.path => conflict_file }).as_json
expect(subject[:diff_files]).to eq(payload)
end
subject[:diff_files]
end
end
end
......
......@@ -7,13 +7,13 @@
let(:request) { double('request', current_user: user) }
let(:merge_request) { create(:merge_request) }
let(:diff_batch) { merge_request.merge_request_diff.diffs_in_batch(2, 3, diff_options: nil) }
let(:allow_tree_conflicts) { false }
let(:merge_conflicts_in_diff) { false }
let(:options) do
{
request: request,
merge_request: merge_request,
pagination_data: diff_batch.pagination_data,
allow_tree_conflicts: allow_tree_conflicts
merge_conflicts_in_diff: merge_conflicts_in_diff
}
end
......@@ -29,61 +29,39 @@
expect(subject[:pagination]).to eq(total_pages: 20)
end
context 'when there are conflicts' do
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 }
describe 'diff_files' do
let(:diff_files) { diff_batch.diff_files(sorted: true) }
let(:resolvable_conflicts) { true }
let(:conflict_file) { double(path: diff_file_with_conflict.new_path, conflict_type: :both_modified) }
let(:conflicts) { double(conflicts: double(files: [conflict_file]), can_be_resolved_in_ui?: resolvable_conflicts) }
it 'serializes diff files using DiffFileEntity' do
expect(DiffFileEntity)
.to receive(:represent)
.with(
diff_files,
hash_including(options.merge(conflicts: nil))
)
let(:merge_ref_head_diff) { true }
let(:options) { super().merge(merge_ref_head_diff: merge_ref_head_diff) }
before do
allow(merge_request).to receive(:cannot_be_merged?).and_return(true)
allow(MergeRequests::Conflicts::ListService).to receive(:new).and_return(conflicts)
end
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 '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
subject[:diff_files]
end
context 'when conflicts cannot be resolved' do
let(:resolvable_conflicts) { false }
context 'when merge_conflicts_in_diff is true' do
let(:conflict_file) { double(path: diff_files.first.new_path, conflict_type: :both_modified) }
let(:conflicts) { double(conflicts: double(files: [conflict_file]), can_be_resolved_in_ui?: false) }
let(:merge_conflicts_in_diff) { true }
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
before do
allow(merge_request).to receive(:cannot_be_merged?).and_return(true)
allow(MergeRequests::Conflicts::ListService).to receive(:new).and_return(conflicts)
end
context 'when allow_tree_conflicts is set to true' do
let(:allow_tree_conflicts) { true }
it 'conflicts are still 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
it 'serializes diff files with conflicts' do
expect(DiffFileEntity)
.to receive(:represent)
.with(
diff_files,
hash_including(options.merge(conflicts: { conflict_file.path => conflict_file }))
)
subject
end
subject[:diff_files]
end
end
end
......
......@@ -190,14 +190,6 @@ def execute_within_threads(amount:, retry_lease: true)
target_branch: 'conflict-start')
end
it 'does not change the merge ref HEAD' do
expect(merge_request.merge_ref_head).to be_nil
subject
expect(merge_request.reload.merge_ref_head).not_to be_nil
end
it 'returns ServiceResponse.error and keeps merge status as cannot_be_merged' do
result = subject
......@@ -351,27 +343,5 @@ def execute_within_threads(amount:, retry_lease: true)
end
end
end
context 'merge with conflicts' do
it 'calls MergeToRefService with true allow_conflicts param' do
expect(MergeRequests::MergeToRefService).to receive(:new)
.with(project: project, current_user: merge_request.author, params: { allow_conflicts: true }).and_call_original
subject
end
context 'when display_merge_conflicts_in_diff is disabled' do
before do
stub_feature_flags(display_merge_conflicts_in_diff: false)
end
it 'calls MergeToRefService with false allow_conflicts param' do
expect(MergeRequests::MergeToRefService).to receive(:new)
.with(project: project, current_user: merge_request.author, params: { allow_conflicts: false }).and_call_original
subject
end
end
end
end
end
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment