From 2eb5c1e99267e00408bd728ffe4c371bf8e3d3be Mon Sep 17 00:00:00 2001
From: Igor Drozdov <idrozdov@gitlab.com>
Date: Mon, 5 Oct 2020 20:13:46 +0300
Subject: [PATCH] Allow conflicts to be merged in diff
When we're calculating merge-ref-head diff, we want the conflicts
to be merged in diff in order to display them differently
---
Gemfile | 2 +-
Gemfile.lock | 4 +--
app/models/merge_request.rb | 2 +-
app/models/repository.rb | 4 +--
.../merge_requests/merge_to_ref_service.rb | 9 ++++-
.../mergeability_check_service.rb | 13 +++++---
.../display_merge_conflicts_in_diff.yml | 7 ++++
lib/gitlab/git/repository.rb | 4 +--
lib/gitlab/gitaly_client/operation_service.rb | 5 +--
spec/lib/gitlab/git/repository_spec.rb | 3 +-
.../gitaly_client/operation_service_spec.rb | 3 +-
spec/models/merge_request_spec.rb | 24 ++++++++++----
.../merge_to_ref_service_spec.rb | 11 +++++++
.../mergeability_check_service_spec.rb | 33 +++++++++++++++++--
14 files changed, 99 insertions(+), 25 deletions(-)
create mode 100644 config/feature_flags/development/display_merge_conflicts_in_diff.yml
diff --git a/Gemfile b/Gemfile
index e5f4930f95e2..4f4cf0753c31 100644
--- a/Gemfile
+++ b/Gemfile
@@ -466,7 +466,7 @@ group :ed25519 do
end
# Gitaly GRPC protocol definitions
-gem 'gitaly', '~> 13.5.0-rc1'
+gem 'gitaly', '~> 13.5.0-rc2'
gem 'grpc', '~> 1.30.2'
diff --git a/Gemfile.lock b/Gemfile.lock
index 415ce44aab18..ac70d988c0f7 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -416,7 +416,7 @@ GEM
rails (>= 3.2.0)
git (1.7.0)
rchardet (~> 1.8)
- gitaly (13.5.0.pre.rc1)
+ gitaly (13.5.0.pre.rc2)
grpc (~> 1.0)
github-markup (1.7.0)
gitlab-chronic (0.10.5)
@@ -1327,7 +1327,7 @@ DEPENDENCIES
gettext (~> 3.3)
gettext_i18n_rails (~> 1.8.0)
gettext_i18n_rails_js (~> 1.3)
- gitaly (~> 13.5.0.pre.rc1)
+ gitaly (~> 13.5.0.pre.rc2)
github-markup (~> 1.7.0)
gitlab-chronic (~> 0.10.5)
gitlab-fog-azure-rm (~> 1.0)
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index ed53676ea977..24541ba32180 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -929,7 +929,7 @@ def check_mergeability(async: false)
# rubocop: enable CodeReuse/ServiceClass
def diffable_merge_ref?
- can_be_merged? && merge_ref_head.present?
+ merge_ref_head.present? && (Feature.enabled?(:display_merge_conflicts_in_diff, project) || can_be_merged?)
end
# Returns boolean indicating the merge_status should be rechecked in order to
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 8142398050df..d4fd202b9668 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -860,10 +860,10 @@ def merge(user, source_sha, merge_request, message)
end
end
- def merge_to_ref(user, source_sha, merge_request, target_ref, message, first_parent_ref)
+ def merge_to_ref(user, source_sha, merge_request, target_ref, message, first_parent_ref, allow_conflicts = false)
branch = merge_request.target_branch
- raw.merge_to_ref(user, source_sha, branch, target_ref, message, first_parent_ref)
+ raw.merge_to_ref(user, source_sha, branch, target_ref, message, first_parent_ref, allow_conflicts)
end
def delete_refs(*ref_names)
diff --git a/app/services/merge_requests/merge_to_ref_service.rb b/app/services/merge_requests/merge_to_ref_service.rb
index 1876b1096fe5..c0115e949039 100644
--- a/app/services/merge_requests/merge_to_ref_service.rb
+++ b/app/services/merge_requests/merge_to_ref_service.rb
@@ -58,8 +58,15 @@ def first_parent_ref
params[:first_parent_ref] || merge_request.target_branch_ref
end
+ ##
+ # The parameter `allow_conflicts` is a flag whether merge conflicts should be merged into diff
+ # Default is false
+ def allow_conflicts
+ params[:allow_conflicts] || false
+ end
+
def commit
- repository.merge_to_ref(current_user, source, merge_request, target_ref, commit_message, first_parent_ref)
+ repository.merge_to_ref(current_user, source, merge_request, target_ref, commit_message, first_parent_ref, allow_conflicts)
rescue Gitlab::Git::PreReceiveError, Gitlab::Git::CommandError => error
raise MergeError, error.message
end
diff --git a/app/services/merge_requests/mergeability_check_service.rb b/app/services/merge_requests/mergeability_check_service.rb
index b41a5fa317e0..627c747203cf 100644
--- a/app/services/merge_requests/mergeability_check_service.rb
+++ b/app/services/merge_requests/mergeability_check_service.rb
@@ -115,10 +115,14 @@ def merge_ref_head_payload
def update_merge_status
return unless merge_request.recheck_merge_status?
+ return merge_request.mark_as_unmergeable if merge_request.broken?
- if can_git_merge? && merge_to_ref
+ merge_to_ref_success = merge_to_ref
+
+ update_diff_discussion_positions! if merge_to_ref_success
+
+ if merge_to_ref_success && can_git_merge?
merge_request.mark_as_mergeable
- update_diff_discussion_positions!
else
merge_request.mark_as_unmergeable
end
@@ -149,13 +153,14 @@ def outdated_merge_ref?
end
def can_git_merge?
- !merge_request.broken? && repository.can_be_merged?(merge_request.diff_head_sha, merge_request.target_branch)
+ repository.can_be_merged?(merge_request.diff_head_sha, merge_request.target_branch)
end
def merge_to_ref
return true unless merge_ref_auto_sync_enabled?
- result = MergeRequests::MergeToRefService.new(project, merge_request.author).execute(merge_request)
+ params = { allow_conflicts: Feature.enabled?(:display_merge_conflicts_in_diff, project) }
+ result = MergeRequests::MergeToRefService.new(project, merge_request.author, params).execute(merge_request)
result[:status] == :success
end
diff --git a/config/feature_flags/development/display_merge_conflicts_in_diff.yml b/config/feature_flags/development/display_merge_conflicts_in_diff.yml
new file mode 100644
index 000000000000..678037fa3666
--- /dev/null
+++ b/config/feature_flags/development/display_merge_conflicts_in_diff.yml
@@ -0,0 +1,7 @@
+---
+name: display_merge_conflicts_in_diff
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/45008
+rollout_issue_url:
+type: development
+group: group::source code
+default_enabled: false
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index 5da85570a3b3..1a3409c1f84e 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -587,9 +587,9 @@ def find_tag(name)
tags.find { |tag| tag.name == name }
end
- def merge_to_ref(user, source_sha, branch, target_ref, message, first_parent_ref)
+ def merge_to_ref(user, source_sha, branch, target_ref, message, first_parent_ref, allow_conflicts)
wrapped_gitaly_errors do
- gitaly_operation_client.user_merge_to_ref(user, source_sha, branch, target_ref, message, first_parent_ref)
+ gitaly_operation_client.user_merge_to_ref(user, source_sha, branch, target_ref, message, first_parent_ref, allow_conflicts)
end
end
diff --git a/lib/gitlab/gitaly_client/operation_service.rb b/lib/gitlab/gitaly_client/operation_service.rb
index 786eb3ca4ae2..4850d646de4a 100644
--- a/lib/gitlab/gitaly_client/operation_service.rb
+++ b/lib/gitlab/gitaly_client/operation_service.rb
@@ -102,7 +102,7 @@ def user_delete_branch(branch_name, user)
end
end
- def user_merge_to_ref(user, source_sha, branch, target_ref, message, first_parent_ref)
+ def user_merge_to_ref(user, source_sha, branch, target_ref, message, first_parent_ref, allow_conflicts)
request = Gitaly::UserMergeToRefRequest.new(
repository: @gitaly_repo,
source_sha: source_sha,
@@ -110,7 +110,8 @@ def user_merge_to_ref(user, source_sha, branch, target_ref, message, first_paren
target_ref: encode_binary(target_ref),
user: Gitlab::Git::User.from_gitlab(user).to_gitaly,
message: encode_binary(message),
- first_parent_ref: encode_binary(first_parent_ref)
+ first_parent_ref: encode_binary(first_parent_ref),
+ allow_conflicts: allow_conflicts
)
response = GitalyClient.call(@repository.storage, :operation_service,
diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb
index 1c818bd4a88b..6dfa791f70b4 100644
--- a/spec/lib/gitlab/git/repository_spec.rb
+++ b/spec/lib/gitlab/git/repository_spec.rb
@@ -1650,13 +1650,14 @@ def commit_files(commit)
let(:right_branch) { 'test-master' }
let(:first_parent_ref) { 'refs/heads/test-master' }
let(:target_ref) { 'refs/merge-requests/999/merge' }
+ let(:allow_conflicts) { false }
before do
repository.create_branch(right_branch, branch_head) unless repository.ref_exists?(first_parent_ref)
end
def merge_to_ref
- repository.merge_to_ref(user, left_sha, right_branch, target_ref, 'Merge message', first_parent_ref)
+ repository.merge_to_ref(user, left_sha, right_branch, target_ref, 'Merge message', first_parent_ref, allow_conflicts)
end
it 'generates a commit in the target_ref' do
diff --git a/spec/lib/gitlab/gitaly_client/operation_service_spec.rb b/spec/lib/gitlab/gitaly_client/operation_service_spec.rb
index b974f456914d..ce01566b8704 100644
--- a/spec/lib/gitlab/gitaly_client/operation_service_spec.rb
+++ b/spec/lib/gitlab/gitaly_client/operation_service_spec.rb
@@ -88,9 +88,10 @@
let(:source_sha) { 'cfe32cf61b73a0d5e9f13e774abde7ff789b1660' }
let(:ref) { 'refs/merge-requests/x/merge' }
let(:message) { 'validación' }
+ let(:allow_conflicts) { false }
let(:response) { Gitaly::UserMergeToRefResponse.new(commit_id: 'new-commit-id') }
- subject { client.user_merge_to_ref(user, source_sha, nil, ref, message, first_parent_ref) }
+ subject { client.user_merge_to_ref(user, source_sha, nil, ref, message, first_parent_ref, allow_conflicts) }
it 'sends a user_merge_to_ref message' do
expect_any_instance_of(Gitaly::OperationService::Stub)
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index b562e41ea2d5..ddb3ffdda2f1 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -4228,14 +4228,26 @@ def create_build(pipeline, coverage, name)
it 'returns true' do
expect(subject.diffable_merge_ref?).to eq(true)
end
- end
- end
- context 'merge request cannot be merged' do
- it 'returns false' do
- subject.mark_as_unchecked!
+ context 'merge request cannot be merged' do
+ before do
+ subject.mark_as_unchecked!
+ end
+
+ it 'returns false' do
+ expect(subject.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
- expect(subject.diffable_merge_ref?).to eq(false)
+ it 'returns false' do
+ expect(subject.diffable_merge_ref?).to eq(false)
+ end
+ end
+ end
end
end
end
diff --git a/spec/services/merge_requests/merge_to_ref_service_spec.rb b/spec/services/merge_requests/merge_to_ref_service_spec.rb
index b482e8d6724b..14ef5b0b772c 100644
--- a/spec/services/merge_requests/merge_to_ref_service_spec.rb
+++ b/spec/services/merge_requests/merge_to_ref_service_spec.rb
@@ -252,5 +252,16 @@ def process_merge_to_ref
end
end
end
+
+ context 'allow conflicts to be merged in diff' do
+ let(:params) { { allow_conflicts: true } }
+
+ it 'calls merge_to_ref with allow_conflicts param' do
+ expect(project.repository).to receive(:merge_to_ref)
+ .with(anything, anything, anything, anything, anything, anything, true)
+
+ service.execute(merge_request)
+ end
+ end
end
end
diff --git a/spec/services/merge_requests/mergeability_check_service_spec.rb b/spec/services/merge_requests/mergeability_check_service_spec.rb
index bffd4c4d34d3..725fc16fa7cd 100644
--- a/spec/services/merge_requests/mergeability_check_service_spec.rb
+++ b/spec/services/merge_requests/mergeability_check_service_spec.rb
@@ -211,11 +211,18 @@ def execute_within_threads(amount:, retry_lease: true)
target_branch: 'conflict-start')
end
- it_behaves_like 'unmergeable merge request'
+ it 'does not change the merge ref HEAD' do
+ expect(merge_request.merge_ref_head).to be_nil
- it 'returns ServiceResponse.error' do
+ 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
+ expect(merge_request.merge_status).to eq('cannot_be_merged')
expect(result).to be_a(ServiceResponse)
expect(result.error?).to be(true)
expect(result.message).to eq('Merge request is not mergeable')
@@ -373,5 +380,27 @@ 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, merge_request.author, { 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, merge_request.author, { allow_conflicts: false }).and_call_original
+
+ subject
+ end
+ end
+ end
end
end
--
GitLab