Commit f8d6f732 authored by Shinya Maeda's avatar Shinya Maeda

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.
parent c03f23c4
...@@ -1127,6 +1127,19 @@ class MergeRequest < ApplicationRecord ...@@ -1127,6 +1127,19 @@ class MergeRequest < ApplicationRecord
"refs/#{Repository::REF_MERGE_REQUEST}/#{iid}/merge" "refs/#{Repository::REF_MERGE_REQUEST}/#{iid}/merge"
end 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) def self.merge_request_ref?(ref)
ref.start_with?("refs/#{Repository::REF_MERGE_REQUEST}/") ref.start_with?("refs/#{Repository::REF_MERGE_REQUEST}/")
end end
......
...@@ -845,6 +845,10 @@ class Repository ...@@ -845,6 +845,10 @@ class Repository
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)
end end
def delete_refs(*ref_names)
raw.delete_refs(*ref_names)
end
def ff_merge(user, source, target_branch, merge_request: nil) def ff_merge(user, source, target_branch, merge_request: nil)
their_commit_id = commit(source)&.id their_commit_id = commit(source)&.id
raise 'Invalid merge source' if their_commit_id.nil? raise 'Invalid merge source' if their_commit_id.nil?
......
...@@ -3220,4 +3220,34 @@ describe MergeRequest do ...@@ -3220,4 +3220,34 @@ describe MergeRequest do
it { is_expected.to be_truthy } it { is_expected.to be_truthy }
end end
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 end
...@@ -191,6 +191,15 @@ describe MergeRequests::MergeToRefService do ...@@ -191,6 +191,15 @@ describe MergeRequests::MergeToRefService do
it { expect(todo).not_to be_done } it { expect(todo).not_to be_done }
end 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 describe 'cascading merge refs' do
set(:project) { create(:project, :repository) } set(:project) { create(:project, :repository) }
let(:params) { { commit_message: 'Cascading merge', first_parent_ref: first_parent_ref, target_ref: target_ref } } let(:params) { { commit_message: 'Cascading merge', first_parent_ref: first_parent_ref, target_ref: target_ref } }
......
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