Commit 50f2fdc7 authored by Mayra Cabrera's avatar Mayra Cabrera

Merge branch '333687-allow-missing-side-list-conflicts' into 'master'

Allow missing side when listing conflicts on diffs

See merge request gitlab-org/gitlab!65516
parents c9931b84 e114784c
......@@ -472,7 +472,7 @@ end
gem 'spamcheck', '~> 0.1.0'
# Gitaly GRPC protocol definitions
gem 'gitaly', '~> 14.1.0.pre.rc3'
gem 'gitaly', '~> 14.1.0.pre.rc4'
# KAS GRPC protocol definitions
gem 'kas-grpc', '~> 0.0.2'
......
......@@ -460,7 +460,7 @@ GEM
rails (>= 3.2.0)
git (1.7.0)
rchardet (~> 1.8)
gitaly (14.1.0.pre.rc3)
gitaly (14.1.0.pre.rc4)
grpc (~> 1.0)
github-markup (1.7.0)
gitlab (4.16.1)
......@@ -531,8 +531,8 @@ GEM
signet (~> 0.12)
google-cloud-env (1.5.0)
faraday (>= 0.17.3, < 2.0)
google-protobuf (3.17.1)
googleapis-common-protos-types (1.0.6)
google-protobuf (3.17.3)
googleapis-common-protos-types (1.1.0)
google-protobuf (~> 3.14)
googleauth (0.14.0)
faraday (>= 0.17.3, < 2.0)
......@@ -1482,7 +1482,7 @@ DEPENDENCIES
gettext (~> 3.3)
gettext_i18n_rails (~> 1.8.0)
gettext_i18n_rails_js (~> 1.3)
gitaly (~> 14.1.0.pre.rc3)
gitaly (~> 14.1.0.pre.rc4)
github-markup (~> 1.7.0)
gitlab-chronic (~> 0.10.5)
gitlab-dangerfiles (~> 2.2.2)
......
......@@ -38,7 +38,8 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
merge_request: @merge_request,
diff_view: diff_view,
merge_ref_head_diff: render_merge_ref_head_diff?,
pagination_data: diffs.pagination_data
pagination_data: diffs.pagination_data,
allow_tree_conflicts: display_merge_conflicts_in_diff?
}
if diff_options_hash[:paths].blank? && Feature.enabled?(:diffs_batch_render_cached, project, default_enabled: :yaml)
......@@ -82,7 +83,8 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
options = additional_attributes.merge(
diff_view: "inline",
merge_ref_head_diff: render_merge_ref_head_diff?
merge_ref_head_diff: render_merge_ref_head_diff?,
allow_tree_conflicts: display_merge_conflicts_in_diff?
)
if @merge_request.project.context_commits_enabled?
......@@ -213,4 +215,8 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter
.track_mr_diffs_single_file_action(merge_request: @merge_request, user: current_user)
end
def display_merge_conflicts_in_diff?
Feature.enabled?(:display_merge_conflicts_in_diff, @merge_request.project)
end
end
......@@ -273,14 +273,14 @@ module DiffHelper
Gitlab::CodeNavigationPath.new(merge_request.project, merge_request.diff_head_sha)
end
def conflicts
def conflicts(allow_tree_conflicts: false)
return unless options[:merge_ref_head_diff]
conflicts_service = MergeRequests::Conflicts::ListService.new(merge_request) # rubocop:disable CodeReuse/ServiceClass
conflicts_service = MergeRequests::Conflicts::ListService.new(merge_request, allow_tree_conflicts: allow_tree_conflicts) # rubocop:disable CodeReuse/ServiceClass
return unless conflicts_service.can_be_resolved_in_ui?
return unless allow_tree_conflicts || conflicts_service.can_be_resolved_in_ui?
conflicts_service.conflicts.files.index_by(&:our_path)
conflicts_service.conflicts.files.index_by(&:path)
end
def log_overflow_limits(diff_files:, collection_overflow:)
......
......@@ -71,7 +71,12 @@ class DiffsEntity < Grape::Entity
submodule_links = Gitlab::SubmoduleLinks.new(merge_request.project.repository)
DiffFileEntity.represent(diffs.diff_files,
options.merge(submodule_links: submodule_links, code_navigation_path: code_navigation_path(diffs), conflicts: conflicts))
options.merge(
submodule_links: submodule_links,
code_navigation_path: code_navigation_path(diffs),
conflicts: conflicts(allow_tree_conflicts: options[:allow_tree_conflicts])
)
)
end
expose :merge_request_diffs, using: MergeRequestDiffEntity, if: -> (_, options) { options[:merge_request_diffs]&.any? } do |diffs|
......
......@@ -17,7 +17,7 @@ class PaginatedDiffEntity < Grape::Entity
options.merge(
submodule_links: submodule_links,
code_navigation_path: code_navigation_path(diffs),
conflicts: conflicts
conflicts: conflicts(allow_tree_conflicts: options[:allow_tree_conflicts])
)
)
end
......
......@@ -3,10 +3,11 @@
module MergeRequests
module Conflicts
class BaseService
attr_reader :merge_request
attr_reader :merge_request, :params
def initialize(merge_request)
def initialize(merge_request, params = {})
@merge_request = merge_request
@params = params
end
end
end
......
......@@ -23,7 +23,11 @@ module MergeRequests
end
def conflicts
@conflicts ||= Gitlab::Conflict::FileCollection.new(merge_request)
@conflicts ||=
Gitlab::Conflict::FileCollection.new(
merge_request,
allow_tree_conflicts: params[:allow_tree_conflicts]
)
end
end
end
......
......@@ -23,7 +23,7 @@ module Gitlab
# 'raw' holds the Gitlab::Git::Conflict::File that this instance wraps
attr_reader :raw
delegate :type, :content, :their_path, :our_path, :our_mode, :our_blob, :repository, to: :raw
delegate :type, :content, :path, :their_path, :our_path, :our_mode, :our_blob, :repository, to: :raw
def initialize(raw, merge_request:)
@raw = raw
......
......@@ -7,14 +7,14 @@ module Gitlab
attr_reader :merge_request, :resolver
def initialize(merge_request)
def initialize(merge_request, allow_tree_conflicts: false)
our_commit = merge_request.source_branch_head.raw
their_commit = merge_request.target_branch_head.raw
@target_repo = merge_request.target_project.repository
@source_repo = merge_request.source_project.repository.raw
@our_commit_id = our_commit.id
@their_commit_id = their_commit.id
@resolver = Gitlab::Git::Conflict::Resolver.new(@target_repo.raw, @our_commit_id, @their_commit_id)
@resolver = Gitlab::Git::Conflict::Resolver.new(@target_repo.raw, @our_commit_id, @their_commit_id, allow_tree_conflicts: allow_tree_conflicts)
@merge_request = merge_request
end
......
......@@ -94,6 +94,15 @@ module Gitlab
resolution
end
def path
# There are conflict scenarios (e.g. file is removed on source) wherein
# our_path will be blank/nil. Since we are indexing them by path in
# `#conflicts` helper and we want to match the diff file to a conflict
# in `DiffFileEntity#highlighted_diff_lines`, we need to fallback to
# their_path (this is the path on target).
our_path.presence || their_path
end
end
end
end
......
......@@ -9,15 +9,16 @@ module Gitlab
ConflictSideMissing = Class.new(StandardError)
ResolutionError = Class.new(StandardError)
def initialize(target_repository, our_commit_oid, their_commit_oid)
def initialize(target_repository, our_commit_oid, their_commit_oid, allow_tree_conflicts: false)
@target_repository = target_repository
@our_commit_oid = our_commit_oid
@their_commit_oid = their_commit_oid
@allow_tree_conflicts = allow_tree_conflicts
end
def conflicts
@conflicts ||= wrapped_gitaly_errors do
gitaly_conflicts_client(@target_repository).list_conflict_files.to_a
gitaly_conflicts_client(@target_repository).list_conflict_files(allow_tree_conflicts: @allow_tree_conflicts).to_a
rescue GRPC::FailedPrecondition => e
raise Gitlab::Git::Conflict::Resolver::ConflictSideMissing, e.message
end
......
......@@ -14,11 +14,12 @@ module Gitlab
@their_commit_oid = their_commit_oid
end
def list_conflict_files
def list_conflict_files(allow_tree_conflicts: false)
request = Gitaly::ListConflictFilesRequest.new(
repository: @gitaly_repo,
our_commit_oid: @our_commit_oid,
their_commit_oid: @their_commit_oid
their_commit_oid: @their_commit_oid,
allow_tree_conflicts: allow_tree_conflicts
)
response = GitalyClient.call(@repository.storage, :conflicts_service, :list_conflict_files, request, timeout: GitalyClient.long_timeout)
GitalyClient::ConflictFilesStitcher.new(response, @gitaly_repo)
......
......@@ -471,6 +471,7 @@ RSpec.describe Projects::MergeRequests::DiffsController do
merge_request: merge_request,
diff_view: :inline,
merge_ref_head_diff: nil,
allow_tree_conflicts: true,
pagination_data: {
total_pages: nil
}.merge(pagination_data)
......@@ -589,6 +590,21 @@ RSpec.describe Projects::MergeRequests::DiffsController do
it_behaves_like 'successful request'
end
context 'when display_merge_conflicts_in_diff is disabled' do
before do
stub_feature_flags(display_merge_conflicts_in_diff: false)
end
subject { go }
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) }
end
it_behaves_like 'successful request'
end
it_behaves_like 'forked project with submodules'
it_behaves_like 'cached diff collection'
......
......@@ -17,6 +17,10 @@ RSpec.describe Gitlab::Conflict::File do
let(:raw_conflict_file) { Gitlab::Git::Conflict::File.new(repository, our_commit.oid, rugged_conflict, raw_conflict_content) }
let(:conflict_file) { described_class.new(raw_conflict_file, merge_request: merge_request) }
describe 'delegates' do
it { expect(conflict_file).to delegate_method(:path).to(:raw) }
end
describe '#resolve_lines' do
let(:section_keys) { conflict_file.sections.map { |section| section[:id] }.compact }
......
......@@ -48,4 +48,18 @@ RSpec.describe Gitlab::Git::Conflict::File do
end
end
end
describe '#path' do
it 'returns our_path' do
expect(valid_content.path).to eq(conflict[:ours][:path])
end
context 'when our_path is not present' do
let(:conflict) { { ancestor: { path: 'ancestor' }, theirs: { path: 'theirs', mode: 33188 }, ours: { path: '', mode: 0 } } }
it 'returns their_path' do
expect(valid_content.path).to eq(conflict[:theirs][:path])
end
end
end
end
......@@ -15,18 +15,31 @@ RSpec.describe Gitlab::GitalyClient::ConflictsService do
end
describe '#list_conflict_files' do
let(:allow_tree_conflicts) { false }
let(:request) do
Gitaly::ListConflictFilesRequest.new(
repository: target_gitaly_repository, our_commit_oid: our_commit_oid,
their_commit_oid: their_commit_oid
repository: target_gitaly_repository,
our_commit_oid: our_commit_oid,
their_commit_oid: their_commit_oid,
allow_tree_conflicts: allow_tree_conflicts
)
end
it 'sends an RPC request' do
expect_any_instance_of(Gitaly::ConflictsService::Stub).to receive(:list_conflict_files)
.with(request, kind_of(Hash)).and_return([].to_enum)
shared_examples_for 'listing conflicts' do
it 'sends an RPC request' do
expect_any_instance_of(Gitaly::ConflictsService::Stub).to receive(:list_conflict_files)
.with(request, kind_of(Hash)).and_return([].to_enum)
client.list_conflict_files(allow_tree_conflicts: allow_tree_conflicts)
end
end
it_behaves_like 'listing conflicts'
context 'when allow_tree_conflicts is set to true' do
let(:allow_tree_conflicts) { true }
client.list_conflict_files
it_behaves_like 'listing conflicts'
end
end
......
......@@ -33,6 +33,7 @@ RSpec.describe 'Merge Requests Diffs' do
merge_request: merge_request,
diff_view: :inline,
merge_ref_head_diff: nil,
allow_tree_conflicts: true,
pagination_data: {
total_pages: nil
}.merge(pagination_data)
......
......@@ -9,8 +9,14 @@ RSpec.describe DiffsEntity do
let(:request) { EntityRequest.new(project: project, current_user: user) }
let(:merge_request_diffs) { merge_request.merge_request_diffs }
let(:allow_tree_conflicts) { false }
let(:options) do
{ request: request, merge_request: merge_request, merge_request_diffs: merge_request_diffs }
{
request: request,
merge_request: merge_request,
merge_request_diffs: merge_request_diffs,
allow_tree_conflicts: allow_tree_conflicts
}
end
let(:entity) do
......@@ -87,7 +93,7 @@ RSpec.describe DiffsEntity do
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(:conflict_file) { double(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 }
......@@ -123,6 +129,18 @@ RSpec.describe DiffsEntity do
subject
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
subject
end
end
end
end
end
......
......@@ -7,11 +7,13 @@ RSpec.describe PaginatedDiffEntity do
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(:options) do
{
request: request,
merge_request: merge_request,
pagination_data: diff_batch.pagination_data
pagination_data: diff_batch.pagination_data,
allow_tree_conflicts: allow_tree_conflicts
}
end
......@@ -34,7 +36,7 @@ RSpec.describe PaginatedDiffEntity do
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(:conflict_file) { double(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 }
......@@ -70,6 +72,18 @@ RSpec.describe PaginatedDiffEntity do
subject
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
subject
end
end
end
end
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