diff options
author | Shinya Maeda <shinya@gitlab.com> | 2019-07-02 05:01:36 +0300 |
---|---|---|
committer | Shinya Maeda <shinya@gitlab.com> | 2019-07-08 10:36:32 +0300 |
commit | f8d6f7322ea43ff7844d006885db9f0c36ad7cc9 (patch) | |
tree | b1177546270245395ca30479ada1e75da903a4a9 | |
parent | c03f23c48311f0663458bbed33131fa3a6c80842 (diff) |
Fix race condition on merge train ref generation
Today, Pipelines for merge train run on `refs/merge`,
however, this causes a race condition that it can be
overwritten by CheckMergeabilityService.
This patch fixes the problem by generating `refs/train`
for those pipelines.
-rw-r--r-- | app/models/merge_request.rb | 13 | ||||
-rw-r--r-- | app/models/repository.rb | 4 | ||||
-rw-r--r-- | spec/models/merge_request_spec.rb | 30 | ||||
-rw-r--r-- | spec/services/merge_requests/merge_to_ref_service_spec.rb | 9 |
4 files changed, 56 insertions, 0 deletions
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index e96e26cc773..53977748c30 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -1127,6 +1127,19 @@ class MergeRequest < ApplicationRecord "refs/#{Repository::REF_MERGE_REQUEST}/#{iid}/merge" end + def train_ref_path + "refs/#{Repository::REF_MERGE_REQUEST}/#{iid}/train" + end + + def cleanup_refs(only: :all) + target_refs = [] + target_refs << ref_path if %i[all head].include?(only) + target_refs << merge_ref_path if %i[all merge].include?(only) + target_refs << train_ref_path if %i[all train].include?(only) + + project.repository.delete_refs(*target_refs) + end + def self.merge_request_ref?(ref) ref.start_with?("refs/#{Repository::REF_MERGE_REQUEST}/") end diff --git a/app/models/repository.rb b/app/models/repository.rb index a408db7ebbe..a25d5abfa64 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -845,6 +845,10 @@ class Repository raw.merge_to_ref(user, source_sha, branch, target_ref, message, first_parent_ref) end + def delete_refs(*ref_names) + raw.delete_refs(*ref_names) + 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/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index fe6d68aff3f..9b0c232f370 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -3220,4 +3220,34 @@ describe MergeRequest do it { is_expected.to be_truthy } end end + + describe '#cleanup_refs' do + subject { merge_request.cleanup_refs(only: only) } + + let(:merge_request) { build(:merge_request) } + + context 'when removing all refs' do + let(:only) { :all } + + it 'deletes all refs from the target project' do + expect(merge_request.target_project.repository) + .to receive(:delete_refs) + .with(merge_request.ref_path, merge_request.merge_ref_path, merge_request.train_ref_path) + + subject + end + end + + context 'when removing only train ref' do + let(:only) { :train } + + it 'deletes train ref from the target project' do + expect(merge_request.target_project.repository) + .to receive(:delete_refs) + .with(merge_request.train_ref_path) + + subject + 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 e2f201677fa..14012b4ea2d 100644 --- a/spec/services/merge_requests/merge_to_ref_service_spec.rb +++ b/spec/services/merge_requests/merge_to_ref_service_spec.rb @@ -191,6 +191,15 @@ describe MergeRequests::MergeToRefService do it { expect(todo).not_to be_done } end + context 'when target ref is passed as a parameter' do + let(:params) { { commit_message: 'merge train', target_ref: target_ref } } + + it_behaves_like 'successfully merges to ref with merge method' do + let(:first_parent_ref) { 'refs/heads/master' } + let(:target_ref) { 'refs/merge-requests/1/train' } + end + end + describe 'cascading merge refs' do set(:project) { create(:project, :repository) } let(:params) { { commit_message: 'Cascading merge', first_parent_ref: first_parent_ref, target_ref: target_ref } } |