From 9e126a06392558f8f4787cfd55c1bc0f34992c62 Mon Sep 17 00:00:00 2001
From: Hordur Freyr Yngvason <hfyngvason@gitlab.com>
Date: Tue, 11 Mar 2025 07:09:05 -0400
Subject: [PATCH] Fix merge request pipelines on ambiguous refs
When the merge request source branch was ambiguous in the source
project, detached merge request pipelines would pick commit SHA
associated with the tag instead of the merge request's branch.
We fix that by using `source_branch_ref` instead of `source_branch` in
the places deciding the commit used by detached merge request pipelines.
Changelog: fixed
---
app/models/merge_request.rb | 6 +--
.../merge_requests/create_pipeline_service.rb | 2 +-
spec/models/merge_request_spec.rb | 54 ++++++++++++++++---
.../create_pipeline_service_spec.rb | 26 +++++++++
4 files changed, 77 insertions(+), 11 deletions(-)
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index cfea6a5559278..a5ff789d77fa2 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -1035,8 +1035,8 @@ def diff_head_sha
# We use these attributes to force these to the intended values.
attr_writer :target_branch_sha, :source_branch_sha
- def source_branch_ref
- return @source_branch_sha if @source_branch_sha
+ def source_branch_ref(or_sha: true)
+ return @source_branch_sha if @source_branch_sha && or_sha
return unless source_branch
Gitlab::Git::BRANCH_REF_PREFIX + source_branch
@@ -1754,7 +1754,7 @@ def environments_in_head_pipeline(deployment_status: nil)
end
def fetch_ref!
- target_project.repository.fetch_source_branch!(source_project.repository, source_branch, ref_path)
+ target_project.repository.fetch_source_branch!(source_project.repository, source_branch_ref(or_sha: false), ref_path)
expire_ancestor_cache
end
diff --git a/app/services/merge_requests/create_pipeline_service.rb b/app/services/merge_requests/create_pipeline_service.rb
index 0e245d47bd309..cea861aa28c45 100644
--- a/app/services/merge_requests/create_pipeline_service.rb
+++ b/app/services/merge_requests/create_pipeline_service.rb
@@ -52,7 +52,7 @@ def detached_pipeline_project_and_ref(merge_request)
if can_create_pipeline_in_target_project?(merge_request)
[merge_request.target_project, merge_request.ref_path]
else
- [merge_request.source_project, merge_request.source_branch]
+ [merge_request.source_project, merge_request.source_branch_ref(or_sha: false)]
end
end
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 1a0b5692add9f..dc20531722c35 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1489,6 +1489,29 @@ def expected_total_time(mrs)
end
end
+ describe '#source_branch_ref' do
+ using RSpec::Parameterized::TableSyntax
+
+ where(:or_sha, :source_branch, :source_branch_sha, :expected) do
+ true | nil | 'sha' | 'sha'
+ true | 'branch' | 'sha' | 'sha'
+ true | 'branch' | nil | 'refs/heads/branch'
+ false | nil | 'sha' | nil
+ false | 'branch' | 'sha' | 'refs/heads/branch'
+ false | 'branch' | 'sha' | 'refs/heads/branch'
+ end
+
+ with_them do
+ specify do
+ merge_request = build(:merge_request, source_branch:)
+ merge_request.source_branch_sha = source_branch_sha
+ result = merge_request.source_branch_ref(or_sha:)
+
+ expect(result).to eq(expected)
+ end
+ end
+ end
+
describe '#source_branch_sha' do
let(:last_branch_commit) { subject.source_project.repository.commit(Gitlab::Git::BRANCH_REF_PREFIX + subject.source_branch) }
@@ -5009,19 +5032,36 @@ def create_build(pipeline, coverage, name)
end
describe '#fetch_ref!' do
- let(:project) { create(:project, :repository) }
+ let_it_be(:project) { create(:project, :repository) }
+ let(:merge_request) { create(:merge_request, source_project: project) }
- subject { create(:merge_request, source_project: project) }
+ before_all do
+ repository = project.repository
+ repository.add_branch(project.creator, 'ambiguous', 'feature')
+ repository.add_tag(project.creator, 'ambiguous', 'master')
+ end
it 'fetches the ref and expires the ancestor cache' do
- expect { subject.target_project.repository.delete_refs(subject.ref_path) }.not_to raise_error
+ expect { merge_request.target_project.repository.delete_refs(merge_request.ref_path) }.not_to raise_error
- expect(project.repository).to receive(:expire_ancestor_cache).with(subject.target_branch_sha, subject.diff_head_sha).and_call_original
- expect(subject).to receive(:expire_ancestor_cache).and_call_original
+ expect(project.repository).to receive(:expire_ancestor_cache).with(merge_request.target_branch_sha, merge_request.diff_head_sha).and_call_original
+ expect(merge_request).to receive(:expire_ancestor_cache).and_call_original
- subject.fetch_ref!
+ merge_request.fetch_ref!
- expect(subject.target_project.repository.ref_exists?(subject.ref_path)).to be_truthy
+ expect(merge_request.target_project.repository.ref_exists?(merge_request.ref_path)).to be_truthy
+ end
+
+ context 'when source branch is ambiguous' do
+ let(:merge_request) { create(:merge_request, source_project: project, source_branch: 'ambiguous') }
+
+ it 'fetches the branch, not the tag' do
+ merge_request.fetch_ref!
+
+ ref_path_sha = merge_request.target_project.commit(merge_request.ref_path).sha
+ expect(ref_path_sha).to eq(merge_request.source_branch_sha)
+ expect(ref_path_sha).not_to eq(project.commit('refs/tags/ambiguous').sha)
+ end
end
end
diff --git a/spec/services/merge_requests/create_pipeline_service_spec.rb b/spec/services/merge_requests/create_pipeline_service_spec.rb
index ecd233cf0c345..b68412962bd56 100644
--- a/spec/services/merge_requests/create_pipeline_service_spec.rb
+++ b/spec/services/merge_requests/create_pipeline_service_spec.rb
@@ -47,6 +47,32 @@
expect(response.payload).to be_detached_merge_request_pipeline
end
+ context 'when ref is ambiguous' do
+ let(:merge_request) do
+ create(:merge_request,
+ source_branch: 'ambiguous',
+ source_project: source_project,
+ target_branch: 'master',
+ target_project: project)
+ end
+
+ before do
+ repository = source_project.repository
+ repository.add_branch(user, 'ambiguous', 'feature')
+ repository.add_tag(user, 'ambiguous', 'master')
+ end
+
+ it 'creates a detached merge request pipeline for the branch, not the tag', :aggregate_failures do
+ expect { response }.to change { Ci::Pipeline.count }.by(1)
+
+ expect(response).to be_success
+ expect(response.payload).to be_persisted
+ expect(response.payload).to be_detached_merge_request_pipeline
+ expect(response.payload.sha).to eq merge_request.source_branch_sha
+ expect(response.payload.sha).not_to eq(source_project.commit('refs/tags/ambiguous').sha)
+ end
+ end
+
it 'defaults to merge_request_event' do
expect(response.payload.source).to eq('merge_request_event')
end
--
GitLab