From 2e4e93a39e7afe24ecd0718cf7546526c74a18e3 Mon Sep 17 00:00:00 2001
From: Oswaldo Ferreira <oswaldo@gitlab.com>
Date: Thu, 31 Jan 2019 16:32:44 -0200
Subject: [PATCH] Support merge to ref for merge-commit and squash
Adds the ground work for writing into
the merge ref refs/merge-requests/:iid/merge the
merge result between source and target branches of
a MR, without further side-effects such as
mailing, MR updates and target branch changes.
---
GITALY_SERVER_VERSION | 2 +-
Gemfile | 3 +-
Gemfile.lock | 4 +-
app/models/merge_request.rb | 14 ++
app/models/project.rb | 8 +
app/models/repository.rb | 6 +
.../merge_requests/merge_base_service.rb | 65 ++++++
app/services/merge_requests/merge_service.rb | 60 ++----
.../merge_requests/merge_to_ref_service.rb | 76 +++++++
...osw-create-and-store-merge-ref-for-mrs.yml | 5 +
...merge_service.rb => merge_base_service.rb} | 44 ++--
.../merge_requests/merge_service_spec.rb | 82 +------
.../merge_to_ref_service_spec.rb | 35 +++
.../services/merge_merge_requests.rb | 98 +++++++++
lib/gitlab/git/repository.rb | 6 +
lib/gitlab/gitaly_client/operation_service.rb | 19 ++
spec/lib/gitlab/git/repository_spec.rb | 31 +++
spec/models/repository_spec.rb | 23 ++
.../merge_requests/merge_service_spec.rb | 12 ++
.../merge_to_ref_service_spec.rb | 201 ++++++++++++++++++
20 files changed, 649 insertions(+), 145 deletions(-)
create mode 100644 app/services/merge_requests/merge_base_service.rb
create mode 100644 app/services/merge_requests/merge_to_ref_service.rb
create mode 100644 changelogs/unreleased/osw-create-and-store-merge-ref-for-mrs.yml
rename ee/app/services/ee/merge_requests/{merge_service.rb => merge_base_service.rb} (50%)
create mode 100644 ee/spec/services/merge_requests/merge_to_ref_service_spec.rb
create mode 100644 ee/spec/support/shared_examples/services/merge_merge_requests.rb
create mode 100644 spec/services/merge_requests/merge_to_ref_service_spec.rb
diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION
index 3989355915568..3500250a4b05b 100644
--- a/GITALY_SERVER_VERSION
+++ b/GITALY_SERVER_VERSION
@@ -1 +1 @@
-1.20.0
+1.21.0
diff --git a/Gemfile b/Gemfile
index c68c769ef3d25..ce6cec8229ce1 100644
--- a/Gemfile
+++ b/Gemfile
@@ -434,7 +434,8 @@ group :ed25519 do
end
# Gitaly GRPC client
-gem 'gitaly-proto', '~> 1.10.0', require: 'gitaly'
+gem 'gitaly-proto', '~> 1.12.0', require: 'gitaly'
+
gem 'grpc', '~> 1.15.0'
gem 'google-protobuf', '~> 3.6'
diff --git a/Gemfile.lock b/Gemfile.lock
index cfb8a42dc1537..532a76322fbe6 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -302,7 +302,7 @@ GEM
gettext_i18n_rails (>= 0.7.1)
po_to_json (>= 1.0.0)
rails (>= 3.2.0)
- gitaly-proto (1.10.0)
+ gitaly-proto (1.12.0)
grpc (~> 1.0)
github-markup (1.7.0)
gitlab-default_value_for (3.1.1)
@@ -1052,7 +1052,7 @@ DEPENDENCIES
gettext (~> 3.2.2)
gettext_i18n_rails (~> 1.8.0)
gettext_i18n_rails_js (~> 1.3)
- gitaly-proto (~> 1.10.0)
+ gitaly-proto (~> 1.12.0)
github-markup (~> 1.7.0)
gitlab-default_value_for (~> 3.1.1)
gitlab-license (~> 1.0)
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index f02c29a733a02..2783b309a9893 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -766,6 +766,16 @@ def mergeable_state?(skip_ci_check: false, skip_discussions_check: false)
true
end
+ def mergeable_to_ref?
+ return false if merged?
+ return false if broken?
+
+ # Given the `merge_ref_path` will have the same
+ # state the `target_branch` would have. Ideally
+ # we need to check if it can be merged to it.
+ project.repository.can_be_merged?(diff_head_sha, target_branch)
+ end
+
def ff_merge_possible?
project.repository.ancestor?(target_branch_sha, diff_head_sha)
end
@@ -1079,6 +1089,10 @@ def ref_path
"refs/#{Repository::REF_MERGE_REQUEST}/#{iid}/head"
end
+ def merge_ref_path
+ "refs/#{Repository::REF_MERGE_REQUEST}/#{iid}/merge"
+ end
+
def in_locked_state
begin
lock_mr
diff --git a/app/models/project.rb b/app/models/project.rb
index 7607fd4ca17fe..0ae75e217f94c 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1925,6 +1925,14 @@ def renamed?
persisted? && path_changed?
end
+ def human_merge_method
+ if merge_method == :ff
+ 'Fast-forward'
+ else
+ merge_method.to_s.humanize
+ end
+ end
+
def merge_method
if self.merge_requests_ff_only_enabled
:ff
diff --git a/app/models/repository.rb b/app/models/repository.rb
index f94a09b67fe19..2fb4ca79e6b76 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -854,6 +854,12 @@ def merge(user, source_sha, merge_request, message)
end
end
+ def merge_to_ref(user, source_sha, merge_request, target_ref, message)
+ branch = merge_request.target_branch
+
+ raw.merge_to_ref(user, source_sha, branch, target_ref, message)
+ end
+
def ff_merge(user, source, target_branch, merge_request: nil)
their_commit_id = commit(source)&.id
raise 'Invalid merge source' if their_commit_id.nil?
diff --git a/app/services/merge_requests/merge_base_service.rb b/app/services/merge_requests/merge_base_service.rb
new file mode 100644
index 0000000000000..2c17725493761
--- /dev/null
+++ b/app/services/merge_requests/merge_base_service.rb
@@ -0,0 +1,65 @@
+# frozen_string_literal: true
+
+module MergeRequests
+ class MergeBaseService < MergeRequests::BaseService
+ include Gitlab::Utils::StrongMemoize
+
+ MergeError = Class.new(StandardError)
+
+ attr_reader :merge_request
+
+ # Overridden in EE.
+ def hooks_validation_pass?(_merge_request)
+ true
+ end
+
+ # Overridden in EE.
+ def hooks_validation_error(_merge_request)
+ # No-op
+ end
+
+ def source
+ if merge_request.squash
+ squash_sha!
+ else
+ merge_request.diff_head_sha
+ end
+ end
+
+ private
+
+ # Overridden in EE.
+ def error_check!
+ # No-op
+ end
+
+ def raise_error(message)
+ raise MergeError, message
+ end
+
+ def handle_merge_error(*args)
+ # No-op
+ end
+
+ def commit_message
+ params[:commit_message] ||
+ merge_request.default_merge_commit_message
+ end
+
+ def squash_sha!
+ strong_memoize(:squash_sha) do
+ params[:merge_request] = merge_request
+ squash_result = ::MergeRequests::SquashService.new(project, current_user, params).execute
+
+ case squash_result[:status]
+ when :success
+ squash_result[:squash_sha]
+ when :error
+ raise ::MergeRequests::MergeService::MergeError, squash_result[:message]
+ end
+ end
+ end
+ end
+end
+
+MergeRequests::MergeBaseService.prepend(EE::MergeRequests::MergeBaseService)
diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb
index 3deb9198bcca7..8241e408ce517 100644
--- a/app/services/merge_requests/merge_service.rb
+++ b/app/services/merge_requests/merge_service.rb
@@ -7,13 +7,7 @@ module MergeRequests
# mark merge request as merged and execute all hooks and notifications
# Executed when you do merge via GitLab UI
#
- class MergeService < MergeRequests::BaseService
- include Gitlab::Utils::StrongMemoize
-
- MergeError = Class.new(StandardError)
-
- attr_reader :merge_request, :source
-
+ class MergeService < MergeRequests::MergeBaseService
delegate :merge_jid, :state, to: :@merge_request
def execute(merge_request)
@@ -24,7 +18,7 @@ def execute(merge_request)
@merge_request = merge_request
- error_check!
+ validate!
merge_request.in_locked_state do
if commit
@@ -38,22 +32,22 @@ def execute(merge_request)
handle_merge_error(log_message: e.message, save_message_on_model: true)
end
- def source
- if merge_request.squash
- squash_sha!
- else
- merge_request.diff_head_sha
- end
- end
+ private
- # Overridden in EE.
- def hooks_validation_pass?(_merge_request)
- true
+ def validate!
+ authorization_check!
+ error_check!
end
- private
+ def authorization_check!
+ unless @merge_request.can_be_merged_by?(current_user)
+ raise_error('You are not allowed to merge this merge request')
+ end
+ end
def error_check!
+ super
+
error =
if @merge_request.should_be_rebased?
'Only fast-forward merge is allowed for your project. Please update your source branch'
@@ -63,7 +57,7 @@ def error_check!
'No source for merge'
end
- raise MergeError, error if error
+ raise_error(error) if error
end
def commit
@@ -73,36 +67,20 @@ def commit
if commit_id
log_info("Git merge finished on JID #{merge_jid} commit #{commit_id}")
else
- raise MergeError, 'Conflicts detected during merge'
+ raise_error('Conflicts detected during merge')
end
merge_request.update!(merge_commit_sha: commit_id)
end
- def squash_sha!
- strong_memoize(:squash_sha) do
- params[:merge_request] = merge_request
- squash_result = ::MergeRequests::SquashService.new(project, current_user, params).execute
-
- case squash_result[:status]
- when :success
- squash_result[:squash_sha]
- when :error
- raise ::MergeRequests::MergeService::MergeError, squash_result[:message]
- end
- end
- end
-
def try_merge
- message = params[:commit_message] || merge_request.default_merge_commit_message
-
- repository.merge(current_user, source, merge_request, message)
+ repository.merge(current_user, source, merge_request, commit_message)
rescue Gitlab::Git::PreReceiveError => e
handle_merge_error(log_message: e.message)
- raise MergeError, 'Something went wrong during merge pre-receive hook'
+ raise_error('Something went wrong during merge pre-receive hook')
rescue => e
handle_merge_error(log_message: e.message)
- raise MergeError, 'Something went wrong during merge'
+ raise_error('Something went wrong during merge')
ensure
merge_request.update!(in_progress_merge_commit_sha: nil)
end
@@ -149,5 +127,3 @@ def merge_request_info
end
end
end
-
-MergeRequests::MergeService.prepend(EE::MergeRequests::MergeService)
diff --git a/app/services/merge_requests/merge_to_ref_service.rb b/app/services/merge_requests/merge_to_ref_service.rb
new file mode 100644
index 0000000000000..586652ae44e5b
--- /dev/null
+++ b/app/services/merge_requests/merge_to_ref_service.rb
@@ -0,0 +1,76 @@
+# frozen_string_literal: true
+
+module MergeRequests
+ # Performs the merge between source SHA and the target branch. Instead
+ # of writing the result to the MR target branch, it targets the `target_ref`.
+ #
+ # Ideally this should leave the `target_ref` state with the same state the
+ # target branch would have if we used the regular `MergeService`, but without
+ # every side-effect that comes with it (MR updates, mails, source branch
+ # deletion, etc). This service should be kept idempotent (i.e. can
+ # be executed regardless of the `target_ref` current state).
+ #
+ class MergeToRefService < MergeRequests::MergeBaseService
+ def execute(merge_request)
+ @merge_request = merge_request
+
+ validate!
+
+ commit_id = commit
+
+ raise_error('Conflicts detected during merge') unless commit_id
+
+ success(commit_id: commit_id)
+ rescue MergeError => error
+ error(error.message)
+ end
+
+ private
+
+ def validate!
+ authorization_check!
+ error_check!
+ end
+
+ def error_check!
+ super
+
+ error =
+ if Feature.disabled?(:merge_to_tmp_merge_ref_path, project)
+ 'Feature is not enabled'
+ elsif !merge_method_supported?
+ "#{project.human_merge_method} to #{target_ref} is currently not supported."
+ elsif !hooks_validation_pass?(merge_request)
+ hooks_validation_error(merge_request)
+ elsif @merge_request.should_be_rebased?
+ 'Fast-forward merge is not possible. Please update your source branch.'
+ elsif !@merge_request.mergeable_to_ref?
+ "Merge request is not mergeable to #{target_ref}"
+ elsif !source
+ 'No source for merge'
+ end
+
+ raise_error(error) if error
+ end
+
+ def authorization_check!
+ unless Ability.allowed?(current_user, :admin_merge_request, project)
+ raise_error("You are not allowed to merge to this ref")
+ end
+ end
+
+ def target_ref
+ merge_request.merge_ref_path
+ end
+
+ def commit
+ repository.merge_to_ref(current_user, source, merge_request, target_ref, commit_message)
+ rescue Gitlab::Git::PreReceiveError => error
+ raise MergeError, error.message
+ end
+
+ def merge_method_supported?
+ [:merge, :rebase_merge].include?(project.merge_method)
+ end
+ end
+end
diff --git a/changelogs/unreleased/osw-create-and-store-merge-ref-for-mrs.yml b/changelogs/unreleased/osw-create-and-store-merge-ref-for-mrs.yml
new file mode 100644
index 0000000000000..012b547a6304e
--- /dev/null
+++ b/changelogs/unreleased/osw-create-and-store-merge-ref-for-mrs.yml
@@ -0,0 +1,5 @@
+---
+title: Support merge ref writing (without merging to target branch)
+merge_request: 24692
+author:
+type: added
diff --git a/ee/app/services/ee/merge_requests/merge_service.rb b/ee/app/services/ee/merge_requests/merge_base_service.rb
similarity index 50%
rename from ee/app/services/ee/merge_requests/merge_service.rb
rename to ee/app/services/ee/merge_requests/merge_base_service.rb
index ff0a7fbc11ef9..e7f0c914c8524 100644
--- a/ee/app/services/ee/merge_requests/merge_service.rb
+++ b/ee/app/services/ee/merge_requests/merge_base_service.rb
@@ -2,14 +2,12 @@
module EE
module MergeRequests
- module MergeService
+ module MergeBaseService
extend ::Gitlab::Utils::Override
override :error_check!
def error_check!
check_size_limit
-
- super
end
override :hooks_validation_pass?
@@ -18,33 +16,35 @@ def hooks_validation_pass?(merge_request)
# object instead of relying on the order of method calls.
@merge_request = merge_request # rubocop:disable Gitlab/ModuleWithInstanceVariables
- return true if project.merge_requests_ff_only_enabled
- return true unless project.feature_available?(:push_rules)
-
- push_rule = merge_request.project.push_rule
- return true unless push_rule
+ hooks_error = hooks_validation_error(merge_request)
- unless push_rule.commit_message_allowed?(params[:commit_message])
- handle_merge_error(log_message: "Commit message does not follow the pattern '#{push_rule.commit_message_regex}'", save_message_on_model: true)
- return false
- end
+ return true unless hooks_error
- if push_rule.commit_message_blocked?(params[:commit_message])
- handle_merge_error(log_message: "Commit message contains the forbidden pattern '#{push_rule.commit_message_negative_regex}'", save_message_on_model: true)
- return false
- end
-
- unless push_rule.author_email_allowed?(current_user.commit_email)
- handle_merge_error(log_message: "Commit author's email '#{current_user.commit_email}' does not follow the pattern '#{push_rule.author_email_regex}'", save_message_on_model: true)
- return false
- end
+ handle_merge_error(log_message: hooks_error, save_message_on_model: true)
- true
+ false
rescue PushRule::MatchError => e
handle_merge_error(log_message: e.message, save_message_on_model: true)
false
end
+ override :hooks_validation_error
+ def hooks_validation_error(merge_request)
+ return if project.merge_requests_ff_only_enabled
+ return unless project.feature_available?(:push_rules)
+
+ push_rule = merge_request.project.push_rule
+ return unless push_rule
+
+ if !push_rule.commit_message_allowed?(params[:commit_message])
+ "Commit message does not follow the pattern '#{push_rule.commit_message_regex}'"
+ elsif push_rule.commit_message_blocked?(params[:commit_message])
+ "Commit message contains the forbidden pattern '#{push_rule.commit_message_negative_regex}'"
+ elsif !push_rule.author_email_allowed?(current_user.commit_email)
+ "Commit author's email '#{current_user.commit_email}' does not follow the pattern '#{push_rule.author_email_regex}'"
+ end
+ end
+
private
def check_size_limit
diff --git a/ee/spec/services/merge_requests/merge_service_spec.rb b/ee/spec/services/merge_requests/merge_service_spec.rb
index 3d8977661720b..15f41158fd0fa 100644
--- a/ee/spec/services/merge_requests/merge_service_spec.rb
+++ b/ee/spec/services/merge_requests/merge_service_spec.rb
@@ -4,24 +4,21 @@
let(:user) { create(:user) }
let(:merge_request) { create(:merge_request, :simple) }
let(:project) { merge_request.project }
+ let(:service) { described_class.new(project, user, commit_message: 'Awesome message') }
before do
project.add_maintainer(user)
end
describe '#execute' do
- let(:service) { described_class.new(project, user, commit_message: 'Awesome message') }
-
context 'project has exceeded size limit' do
before do
allow(project).to receive(:above_size_limit?).and_return(true)
-
- perform_enqueued_jobs do
- service.execute(merge_request)
- end
end
- it 'returns the correct error message' do
+ it 'persists the correct error message' do
+ service.execute(merge_request)
+
expect(merge_request.merge_error).to include('This merge request cannot be merged')
end
end
@@ -46,74 +43,5 @@
end
end
- describe '#hooks_validation_pass?' do
- shared_examples 'hook validations are skipped when push rules unlicensed' do
- subject { service.hooks_validation_pass?(merge_request) }
-
- before do
- stub_licensed_features(push_rules: false)
- end
-
- it { is_expected.to be_truthy }
- end
-
- let(:service) { described_class.new(project, user, commit_message: 'Awesome message') }
-
- it 'returns true when valid' do
- expect(service.hooks_validation_pass?(merge_request)).to be_truthy
- end
-
- context 'commit message validation for required characters' do
- before do
- allow(project).to receive(:push_rule) { build(:push_rule, commit_message_regex: 'unmatched pattern .*') }
- end
-
- it_behaves_like 'hook validations are skipped when push rules unlicensed'
-
- it 'returns false and saves error when invalid' do
- expect(service.hooks_validation_pass?(merge_request)).to be_falsey
- expect(merge_request.merge_error).not_to be_empty
- end
- end
-
- context 'commit message validation for forbidden characters' do
- before do
- allow(project).to receive(:push_rule) { build(:push_rule, commit_message_negative_regex: '.*') }
- end
-
- it_behaves_like 'hook validations are skipped when push rules unlicensed'
-
- it 'returns false and saves error when invalid' do
- expect(service.hooks_validation_pass?(merge_request)).to be_falsey
- expect(merge_request.merge_error).not_to be_empty
- end
- end
-
- context 'authors email validation' do
- before do
- allow(project).to receive(:push_rule) { build(:push_rule, author_email_regex: '.*@unmatchedemaildomain.com') }
- end
-
- it_behaves_like 'hook validations are skipped when push rules unlicensed'
-
- it 'returns false and saves error when invalid' do
- expect(service.hooks_validation_pass?(merge_request)).to be_falsey
- expect(merge_request.merge_error).not_to be_empty
- end
-
- it 'validates against the commit email' do
- user.commit_email = 'foo@unmatchedemaildomain.com'
-
- expect(service.hooks_validation_pass?(merge_request)).to be_truthy
- end
- end
-
- context 'fast forward merge request' do
- it 'returns true when fast forward is enabled' do
- allow(project).to receive(:merge_requests_ff_only_enabled) { true }
-
- expect(service.hooks_validation_pass?(merge_request)).to be_truthy
- end
- end
- end
+ it_behaves_like 'merge validation hooks', persisted: true
end
diff --git a/ee/spec/services/merge_requests/merge_to_ref_service_spec.rb b/ee/spec/services/merge_requests/merge_to_ref_service_spec.rb
new file mode 100644
index 0000000000000..0308fce5a9b1d
--- /dev/null
+++ b/ee/spec/services/merge_requests/merge_to_ref_service_spec.rb
@@ -0,0 +1,35 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe MergeRequests::MergeToRefService do
+ let(:user) { create(:user) }
+ let(:merge_request) { create(:merge_request, :simple) }
+ let(:project) { merge_request.project }
+ let(:service) { described_class.new(project, user, commit_message: 'Awesome message') }
+
+ before do
+ project.add_maintainer(user)
+ end
+
+ describe '#execute' do
+ context 'project has exceeded size limit' do
+ before do
+ allow(project).to receive(:above_size_limit?).and_return(true)
+ end
+
+ it 'returns the correct error message' do
+ result = service.execute(merge_request)
+
+ expected_error =
+ 'This merge request cannot be merged, because this repository ' \
+ 'has exceeded its size limit'
+
+ expect(result[:status]).to eq(:error)
+ expect(result[:message]).to start_with(expected_error)
+ end
+ end
+ end
+
+ it_behaves_like 'merge validation hooks', persisted: false
+end
diff --git a/ee/spec/support/shared_examples/services/merge_merge_requests.rb b/ee/spec/support/shared_examples/services/merge_merge_requests.rb
new file mode 100644
index 0000000000000..68bc17b3e1532
--- /dev/null
+++ b/ee/spec/support/shared_examples/services/merge_merge_requests.rb
@@ -0,0 +1,98 @@
+# frozen_string_literal: true
+
+shared_examples 'merge validation hooks' do |args|
+ def hooks_error
+ service.hooks_validation_error(merge_request)
+ end
+
+ def hooks_pass?
+ service.hooks_validation_pass?(merge_request)
+ end
+
+ shared_examples 'hook validations are skipped when push rules unlicensed' do
+ subject { service.hooks_validation_pass?(merge_request) }
+
+ before do
+ stub_licensed_features(push_rules: false)
+ end
+
+ it { is_expected.to be_truthy }
+ end
+
+ it 'returns true when valid' do
+ expect(service.hooks_validation_pass?(merge_request)).to be(true)
+ end
+
+ context 'commit message validation for required characters' do
+ before do
+ allow(project).to receive(:push_rule) { build(:push_rule, commit_message_regex: 'unmatched pattern .*') }
+ end
+
+ it_behaves_like 'hook validations are skipped when push rules unlicensed'
+
+ it 'returns false and matches validation error' do
+ expect(hooks_pass?).to be(false)
+ expect(hooks_error).not_to be_empty
+
+ if args[:persisted]
+ expect(merge_request.merge_error).to eq(hooks_error)
+ else
+ expect(merge_request.merge_error).to be_nil
+ end
+ end
+ end
+
+ context 'commit message validation for forbidden characters' do
+ before do
+ allow(project).to receive(:push_rule) { build(:push_rule, commit_message_negative_regex: '.*') }
+ end
+
+ it_behaves_like 'hook validations are skipped when push rules unlicensed'
+
+ it 'returns false and saves error when invalid' do
+ expect(hooks_pass?).to be(false)
+ expect(hooks_error).not_to be_empty
+
+ if args[:persisted]
+ expect(merge_request.merge_error).to eq(hooks_error)
+ else
+ expect(merge_request.merge_error).to be_nil
+ end
+ end
+ end
+
+ context 'authors email validation' do
+ before do
+ allow(project).to receive(:push_rule) { build(:push_rule, author_email_regex: '.*@unmatchedemaildomain.com') }
+ end
+
+ it_behaves_like 'hook validations are skipped when push rules unlicensed'
+
+ it 'returns false and saves error when invalid' do
+ expect(hooks_pass?).to be(false)
+ expect(hooks_error).not_to be_empty
+
+ if args[:persisted]
+ expect(merge_request.merge_error).to eq(hooks_error)
+ else
+ expect(merge_request.merge_error).to be_nil
+ end
+ end
+
+ it 'validates against the commit email' do
+ user.commit_email = 'foo@unmatchedemaildomain.com'
+
+ expect(hooks_pass?).to be(true)
+ expect(hooks_error).to be_nil
+ end
+ end
+
+ context 'fast forward merge request' do
+ it 'returns true when fast forward is enabled' do
+ allow(project).to receive(:merge_requests_ff_only_enabled) { true }
+
+ expect(hooks_pass?).to be(true)
+ expect(hooks_error).to be_nil
+ end
+ end
+end
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index 51d68fa410eb2..6d0c27e23fca0 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -556,6 +556,12 @@ def find_tag(name)
tags.find { |tag| tag.name == name }
end
+ def merge_to_ref(user, source_sha, branch, target_ref, message)
+ wrapped_gitaly_errors do
+ gitaly_operation_client.user_merge_to_ref(user, source_sha, branch, target_ref, message)
+ end
+ end
+
def merge(user, source_sha, target_branch, message, &block)
wrapped_gitaly_errors do
gitaly_operation_client.user_merge_branch(user, source_sha, target_branch, message, &block)
diff --git a/lib/gitlab/gitaly_client/operation_service.rb b/lib/gitlab/gitaly_client/operation_service.rb
index 22d2d149e65ce..d172c798da24d 100644
--- a/lib/gitlab/gitaly_client/operation_service.rb
+++ b/lib/gitlab/gitaly_client/operation_service.rb
@@ -100,6 +100,25 @@ def user_delete_branch(branch_name, user)
end
end
+ def user_merge_to_ref(user, source_sha, branch, target_ref, message)
+ request = Gitaly::UserMergeToRefRequest.new(
+ repository: @gitaly_repo,
+ source_sha: source_sha,
+ branch: encode_binary(branch),
+ target_ref: encode_binary(target_ref),
+ user: Gitlab::Git::User.from_gitlab(user).to_gitaly,
+ message: message
+ )
+
+ response = GitalyClient.call(@repository.storage, :operation_service, :user_merge_to_ref, request)
+
+ if pre_receive_error = response.pre_receive_error.presence
+ raise Gitlab::Git::PreReceiveError, pre_receive_error
+ end
+
+ response.commit_id
+ end
+
def user_merge_branch(user, source_sha, target_branch, message)
request_enum = QueueEnumerator.new
response_enum = GitalyClient.call(
diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb
index 8a9e78ba3c309..b3a728c139ed1 100644
--- a/spec/lib/gitlab/git/repository_spec.rb
+++ b/spec/lib/gitlab/git/repository_spec.rb
@@ -1704,6 +1704,37 @@ def commit_files(commit)
end
end
+ describe '#merge_to_ref' do
+ let(:repository) { mutable_repository }
+ let(:branch_head) { '6d394385cf567f80a8fd85055db1ab4c5295806f' }
+ let(:left_sha) { 'cfe32cf61b73a0d5e9f13e774abde7ff789b1660' }
+ let(:right_branch) { 'test-master' }
+ let(:target_ref) { 'refs/merge-requests/999/merge' }
+
+ before do
+ repository.create_branch(right_branch, branch_head) unless repository.branch_exists?(right_branch)
+ end
+
+ def merge_to_ref
+ repository.merge_to_ref(user, left_sha, right_branch, target_ref, 'Merge message')
+ end
+
+ it 'generates a commit in the target_ref' do
+ expect(repository.ref_exists?(target_ref)).to be(false)
+
+ commit_sha = merge_to_ref
+ ref_head = repository.commit(target_ref)
+
+ expect(commit_sha).to be_present
+ expect(repository.ref_exists?(target_ref)).to be(true)
+ expect(ref_head.id).to eq(commit_sha)
+ end
+
+ it 'does not change the right branch HEAD' do
+ expect { merge_to_ref }.not_to change { repository.find_branch(right_branch).target }
+ end
+ end
+
describe '#merge' do
let(:repository) { mutable_repository }
let(:source_sha) { '913c66a37b4a45b9769037c55c2d238bd0942d2e' }
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index f78760bf567f3..17201d8b90a53 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -1373,6 +1373,29 @@ def merge(repository, user, merge_request, message)
end
end
+ describe '#merge_to_ref' do
+ let(:merge_request) do
+ create(:merge_request, source_branch: 'feature',
+ target_branch: 'master',
+ source_project: project)
+ end
+
+ it 'writes merge of source and target to MR merge_ref_path' do
+ merge_commit_id = repository.merge_to_ref(user,
+ merge_request.diff_head_sha,
+ merge_request,
+ merge_request.merge_ref_path,
+ 'Custom message')
+
+ merge_commit = repository.commit(merge_commit_id)
+
+ expect(merge_commit.message).to eq('Custom message')
+ expect(merge_commit.author_name).to eq(user.name)
+ expect(merge_commit.author_email).to eq(user.commit_email)
+ expect(repository.blob_at(merge_commit.id, 'files/ruby/feature.rb')).to be_present
+ end
+ end
+
describe '#ff_merge' do
before do
repository.add_branch(user, 'ff-target', 'feature~5')
diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb
index 04a62aa454d7a..ede79b87bcc38 100644
--- a/spec/services/merge_requests/merge_service_spec.rb
+++ b/spec/services/merge_requests/merge_service_spec.rb
@@ -224,6 +224,18 @@
expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message))
end
+ it 'logs and saves error if user is not authorized' do
+ unauthorized_user = create(:user)
+ project.add_reporter(unauthorized_user)
+
+ service = described_class.new(project, unauthorized_user)
+
+ service.execute(merge_request)
+
+ expect(merge_request.merge_error)
+ .to eq('You are not allowed to merge this merge request')
+ end
+
it 'logs and saves error if there is an PreReceiveError exception' do
error_message = 'error message'
diff --git a/spec/services/merge_requests/merge_to_ref_service_spec.rb b/spec/services/merge_requests/merge_to_ref_service_spec.rb
new file mode 100644
index 0000000000000..96f2fde711790
--- /dev/null
+++ b/spec/services/merge_requests/merge_to_ref_service_spec.rb
@@ -0,0 +1,201 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe MergeRequests::MergeToRefService do
+ shared_examples_for 'MergeService for target ref' do
+ it 'target_ref has the same state of target branch' do
+ repo = merge_request.target_project.repository
+
+ process_merge_to_ref
+ merge_service.execute(merge_request)
+
+ ref_commits = repo.commits(merge_request.merge_ref_path, limit: 3)
+ target_branch_commits = repo.commits(merge_request.target_branch, limit: 3)
+
+ ref_commits.zip(target_branch_commits).each do |ref_commit, target_branch_commit|
+ expect(ref_commit.parents).to eq(target_branch_commit.parents)
+ end
+ end
+ end
+
+ set(:user) { create(:user) }
+ let(:merge_request) { create(:merge_request, :simple) }
+ let(:project) { merge_request.project }
+
+ before do
+ project.add_maintainer(user)
+ end
+
+ describe '#execute' do
+ let(:service) do
+ described_class.new(project, user,
+ commit_message: 'Awesome message',
+ 'should_remove_source_branch' => true)
+ end
+
+ def process_merge_to_ref
+ perform_enqueued_jobs do
+ service.execute(merge_request)
+ end
+ end
+
+ it 'writes commit to merge ref' do
+ repository = project.repository
+ target_ref = merge_request.merge_ref_path
+
+ expect(repository.ref_exists?(target_ref)).to be(false)
+
+ result = service.execute(merge_request)
+
+ ref_head = repository.commit(target_ref)
+
+ expect(result[:status]).to eq(:success)
+ expect(result[:commit_id]).to be_present
+ expect(repository.ref_exists?(target_ref)).to be(true)
+ expect(ref_head.id).to eq(result[:commit_id])
+ end
+
+ it 'does not send any mail' do
+ expect { process_merge_to_ref }.not_to change { ActionMailer::Base.deliveries.count }
+ end
+
+ it 'does not change the MR state' do
+ expect { process_merge_to_ref }.not_to change { merge_request.state }
+ end
+
+ it 'does not create notes' do
+ expect { process_merge_to_ref }.not_to change { merge_request.notes.count }
+ end
+
+ it 'does not delete the source branch' do
+ expect(DeleteBranchService).not_to receive(:new)
+
+ process_merge_to_ref
+ end
+
+ it 'returns error when feature is disabled' do
+ stub_feature_flags(merge_to_tmp_merge_ref_path: false)
+
+ result = service.execute(merge_request)
+
+ expect(result[:status]).to eq(:error)
+ expect(result[:message]).to eq('Feature is not enabled')
+ end
+
+ it 'returns an error when the failing to process the merge' do
+ allow(project.repository).to receive(:merge_to_ref).and_return(nil)
+
+ result = service.execute(merge_request)
+
+ expect(result[:status]).to eq(:error)
+ expect(result[:message]).to eq('Conflicts detected during merge')
+ end
+
+ context 'commit history comparison with regular MergeService' do
+ let(:merge_ref_service) do
+ described_class.new(project, user, {})
+ end
+
+ let(:merge_service) do
+ MergeRequests::MergeService.new(project, user, {})
+ end
+
+ context 'when merge commit' do
+ it_behaves_like 'MergeService for target ref'
+ end
+
+ context 'when merge commit with squash' do
+ before do
+ merge_request.update!(squash: true, source_branch: 'master', target_branch: 'feature')
+ end
+
+ it_behaves_like 'MergeService for target ref'
+ end
+ end
+
+ context 'merge pre-condition checks' do
+ before do
+ merge_request.project.update!(merge_method: merge_method)
+ end
+
+ context 'when semi-linear merge method' do
+ let(:merge_method) { :rebase_merge }
+
+ it 'return error when MR should be able to fast-forward' do
+ allow(merge_request).to receive(:should_be_rebased?) { true }
+
+ error_message = 'Fast-forward merge is not possible. Please update your source branch.'
+
+ result = service.execute(merge_request)
+
+ expect(result[:status]).to eq(:error)
+ expect(result[:message]).to eq(error_message)
+ end
+ end
+
+ context 'when fast-forward merge method' do
+ let(:merge_method) { :ff }
+
+ it 'returns error' do
+ error_message = "Fast-forward to #{merge_request.merge_ref_path} is currently not supported."
+
+ result = service.execute(merge_request)
+
+ expect(result[:status]).to eq(:error)
+ expect(result[:message]).to eq(error_message)
+ end
+ end
+
+ context 'when MR is not mergeable to ref' do
+ let(:merge_method) { :merge }
+
+ it 'returns error' do
+ allow(merge_request).to receive(:mergeable_to_ref?) { false }
+
+ error_message = "Merge request is not mergeable to #{merge_request.merge_ref_path}"
+
+ result = service.execute(merge_request)
+
+ expect(result[:status]).to eq(:error)
+ expect(result[:message]).to eq(error_message)
+ end
+ end
+ end
+
+ context 'does not close related todos' do
+ let(:merge_request) { create(:merge_request, assignee: user, author: user) }
+ let(:project) { merge_request.project }
+ let!(:todo) do
+ create(:todo, :assigned,
+ project: project,
+ author: user,
+ user: user,
+ target: merge_request)
+ end
+
+ before do
+ allow(service).to receive(:execute_hooks)
+
+ perform_enqueued_jobs do
+ service.execute(merge_request)
+ todo.reload
+ end
+ end
+
+ it { expect(todo).not_to be_done }
+ end
+
+ it 'returns error when user has no authorization to admin the merge request' do
+ unauthorized_user = create(:user)
+ project.add_reporter(unauthorized_user)
+
+ service = described_class.new(project, unauthorized_user)
+
+ result = service.execute(merge_request)
+
+ expect(result[:status]).to eq(:error)
+ expect(result[:message]).to eq('You are not allowed to merge to this ref')
+ end
+ end
+end
--
GitLab