From 6db401e1450b3263acde64648def14b5e61067eb Mon Sep 17 00:00:00 2001 From: Joe Woodward Date: Tue, 11 Nov 2025 23:17:03 +0000 Subject: [PATCH] Do not create pipeline for tags if no target commit exists Git allows users to tag tree and blob objects. Gitlab accepts these objects when being pushed but we do not handle them correctly when triggering pipelines. When dereferenced_target is nil tag_commit ends up pointing to root_ref as that's the value used when calling `project.commit(nil)` This means we trigger a pipeline for the tag but using HEAD SHA of the root ref. This is incorrect, we can't run a pipeline against a single tree or blob so we should skip these instead. We also can't display these in the UI because we don't have the sha of the dereferenced_target. This change ensures we skip pipelines for tags without a dereferenced_target. Related to https://gitlab.com/gitlab-org/gitlab/-/issues/579813+ Changelog: fixed --- app/services/git/base_hooks_service.rb | 11 +-- app/services/git/tag_hooks_service.rb | 10 ++- .../git/process_ref_changes_service_spec.rb | 70 +++++++++++-------- spec/services/git/tag_hooks_service_spec.rb | 34 ++++++++- 4 files changed, 88 insertions(+), 37 deletions(-) diff --git a/app/services/git/base_hooks_service.rb b/app/services/git/base_hooks_service.rb index 11beebdabafda4..e40ec781c64d32 100644 --- a/app/services/git/base_hooks_service.rb +++ b/app/services/git/base_hooks_service.rb @@ -10,7 +10,7 @@ class BaseHooksService < ::BaseService def execute create_events - create_pipelines + create_pipeline execute_project_hooks # Not a hook, but it needs access to the list of changed commits @@ -55,9 +55,8 @@ def removing_ref? Gitlab::Git.blank_ref?(newrev) end - def create_pipelines - return unless params.fetch(:create_pipelines, true) - return if removing_ref? + def create_pipeline + return unless create_pipeline? sidekiq_safe_pipeline_params = pipeline_params.merge(push_options: push_options&.deep_stringify_keys) @@ -66,6 +65,10 @@ def create_pipelines .execute_async(:push, pipeline_options) end + def create_pipeline? + params.fetch(:create_pipelines, true) && !removing_ref? + end + def execute_project_hooks return unless params.fetch(:execute_project_hooks, true) diff --git a/app/services/git/tag_hooks_service.rb b/app/services/git/tag_hooks_service.rb index 4614cfbd180031..05d5e9ef4558e8 100644 --- a/app/services/git/tag_hooks_service.rb +++ b/app/services/git/tag_hooks_service.rb @@ -6,6 +6,14 @@ class TagHooksService < ::Git::BaseHooksService alias_method :removing_tag?, :removing_ref? + # If a tag does not have a dereferenced_target this means it's not + # referencing a commit. This can happen when you create a tag for a tree or + # blob object. We cannot run pipelines against trees and blobs so we skip + # the creation. + def create_pipeline? + super && tag_commit.present? + end + def hook_name :tag_push_hooks end @@ -35,7 +43,7 @@ def tag def tag_commit strong_memoize(:tag_commit) do - project.commit(tag.dereferenced_target) if tag + project.commit(tag.dereferenced_target) if tag&.dereferenced_target end end end diff --git a/spec/services/git/process_ref_changes_service_spec.rb b/spec/services/git/process_ref_changes_service_spec.rb index 0a7b1a1a40fef0..9b9023cae9ac05 100644 --- a/spec/services/git/process_ref_changes_service_spec.rb +++ b/spec/services/git/process_ref_changes_service_spec.rb @@ -19,14 +19,6 @@ def multiple_changes(change, count) end end - let(:changes) do - [ - { index: 0, oldrev: Gitlab::Git::SHA1_BLANK_SHA, newrev: '789012', ref: "#{ref_prefix}/create" }, - { index: 1, oldrev: '123456', newrev: '789012', ref: "#{ref_prefix}/update" }, - { index: 2, oldrev: '123456', newrev: Gitlab::Git::SHA1_BLANK_SHA, ref: "#{ref_prefix}/delete" } - ] - end - before do allow(git_changes).to receive(changes_method).and_return(changes) end @@ -145,7 +137,10 @@ def multiple_changes(change, count) subject.execute # We don't run a pipeline for a deletion - expect(Ci::Pipeline.pluck(:ref)).to contain_exactly('create', 'update') + refs = changes.filter_map do |change| + Gitlab::Git.ref_name(change[:ref]) unless change[:newrev] == Gitlab::Git::SHA1_BLANK_SHA + end + expect(Ci::Pipeline.pluck(:ref)).to match_array(refs) end it "calls Gitlab::AppJsonLogger when a Git push is made" do @@ -244,10 +239,17 @@ def multiple_changes(change, count) end context 'branch changes' do - let(:changes_method) { :branch_changes } - let(:ref_prefix) { 'refs/heads' } - - it_behaves_like 'service for processing ref changes', Git::BranchPushService + it_behaves_like 'service for processing ref changes', Git::BranchPushService do + let(:ref_prefix) { 'refs/heads' } + let(:changes_method) { :branch_changes } + let(:changes) do + [ + { index: 0, oldrev: Gitlab::Git::SHA1_BLANK_SHA, newrev: '789012', ref: "refs/heads/create" }, + { index: 1, oldrev: '123456', newrev: '789012', ref: "refs/heads/update" }, + { index: 2, oldrev: '123456', newrev: Gitlab::Git::SHA1_BLANK_SHA, ref: "refs/heads/delete" } + ] + end + end context 'when there are merge requests associated with branches' do let(:tag_changes) do @@ -258,13 +260,13 @@ def multiple_changes(change, count) let(:branch_changes) do [ - { index: 0, oldrev: Gitlab::Git::SHA1_BLANK_SHA, newrev: '789012', ref: "#{ref_prefix}/create1" }, - { index: 1, oldrev: Gitlab::Git::SHA1_BLANK_SHA, newrev: '789013', ref: "#{ref_prefix}/create2" }, - { index: 2, oldrev: Gitlab::Git::SHA1_BLANK_SHA, newrev: '789014', ref: "#{ref_prefix}/create3" }, - { index: 3, oldrev: '789015', newrev: '789016', ref: "#{ref_prefix}/changed1" }, - { index: 4, oldrev: '789017', newrev: '789018', ref: "#{ref_prefix}/changed2" }, - { index: 5, oldrev: '789019', newrev: Gitlab::Git::SHA1_BLANK_SHA, ref: "#{ref_prefix}/removed1" }, - { index: 6, oldrev: '789020', newrev: Gitlab::Git::SHA1_BLANK_SHA, ref: "#{ref_prefix}/removed2" } + { index: 0, oldrev: Gitlab::Git::SHA1_BLANK_SHA, newrev: '789012', ref: "refs/heads/create1" }, + { index: 1, oldrev: Gitlab::Git::SHA1_BLANK_SHA, newrev: '789013', ref: "refs/heads/create2" }, + { index: 2, oldrev: Gitlab::Git::SHA1_BLANK_SHA, newrev: '789014', ref: "refs/heads/create3" }, + { index: 3, oldrev: '789015', newrev: '789016', ref: "refs/heads/changed1" }, + { index: 4, oldrev: '789017', newrev: '789018', ref: "refs/heads/changed2" }, + { index: 5, oldrev: '789019', newrev: Gitlab::Git::SHA1_BLANK_SHA, ref: "refs/heads/removed1" }, + { index: 6, oldrev: '789020', newrev: Gitlab::Git::SHA1_BLANK_SHA, ref: "refs/heads/removed2" } ] end @@ -286,7 +288,7 @@ def multiple_changes(change, count) user.id, Gitlab::Git::SHA1_BLANK_SHA, '789012', - "#{ref_prefix}/create1", + "refs/heads/create1", { 'gitaly_context' => nil, 'push_options' => nil }).ordered expect(UpdateMergeRequestsWorker).to receive(:perform_async).with( @@ -294,7 +296,7 @@ def multiple_changes(change, count) user.id, Gitlab::Git::SHA1_BLANK_SHA, '789013', - "#{ref_prefix}/create2", + "refs/heads/create2", { 'gitaly_context' => nil, 'push_options' => nil }).ordered expect(UpdateMergeRequestsWorker).to receive(:perform_async).with( @@ -302,7 +304,7 @@ def multiple_changes(change, count) user.id, '789015', '789016', - "#{ref_prefix}/changed1", + "refs/heads/changed1", { 'gitaly_context' => nil, 'push_options' => nil }).ordered expect(UpdateMergeRequestsWorker).to receive(:perform_async).with( @@ -310,7 +312,7 @@ def multiple_changes(change, count) user.id, '789020', Gitlab::Git::SHA1_BLANK_SHA, - "#{ref_prefix}/removed2", + "refs/heads/removed2", { 'gitaly_context' => nil, 'push_options' => nil }).ordered subject.execute @@ -325,7 +327,7 @@ def multiple_changes(change, count) message: "Some pipelines may not have been created because ref count exceeded limit", ref_limit: Gitlab::CurrentSettings.git_push_pipeline_limit, total_ref_count: branch_changes.count + tag_changes.count, - possible_omitted_refs: ["#{ref_prefix}/changed2", "refs/tags/v10.0.0"], + possible_omitted_refs: ["refs/heads/changed2", "refs/tags/v10.0.0"], possible_omitted_ref_count: 2 ) ) @@ -337,9 +339,19 @@ def multiple_changes(change, count) end context 'tag changes' do - let(:changes_method) { :tag_changes } - let(:ref_prefix) { 'refs/tags' } - - it_behaves_like 'service for processing ref changes', Git::TagPushService + it_behaves_like 'service for processing ref changes', Git::TagPushService do + let(:changes_method) { :tag_changes } + let(:ref_prefix) { 'refs/tags' } + let(:changes) do + tags = project.repository.tags + tag_1 = tags.first + tag_2 = tags.second + [ + { index: 0, oldrev: Gitlab::Git::SHA1_BLANK_SHA, newrev: tag_1.target, ref: "refs/tags/#{tag_1.name}" }, + { index: 1, oldrev: '123456', newrev: tag_2.target, ref: "refs/tags/#{tag_2.name}" }, + { index: 2, oldrev: '123456', newrev: Gitlab::Git::SHA1_BLANK_SHA, ref: "refs/tags/delete" } + ] + end + end end end diff --git a/spec/services/git/tag_hooks_service_spec.rb b/spec/services/git/tag_hooks_service_spec.rb index a826866d9e6645..37e8a19fd2f1c4 100644 --- a/spec/services/git/tag_hooks_service_spec.rb +++ b/spec/services/git/tag_hooks_service_spec.rb @@ -48,10 +48,38 @@ project.add_developer(user) end - it "creates a new pipeline", :sidekiq_inline do - expect { service.execute }.to change { Ci::Pipeline.count }.by(1) + context 'when the tag references a commit' do + it "creates a new pipeline", :sidekiq_inline do + expect { service.execute }.to change { Ci::Pipeline.count }.by(1) - expect(Ci::Pipeline.last).to be_push + expect(Ci::Pipeline.last).to be_push + end + end + + context 'when the tag references a blob' do + let(:tag_name) { 'blob' } + + before do + blob_id = project.repository.readme.id + project.repository.add_tag(user, tag_name, blob_id) + end + + it "does not create a pipeline", :sidekiq_inline do + expect { service.execute }.not_to change { Ci::Pipeline.count } + end + end + + context 'when the tag references a tree' do + let(:tag_name) { 'tree' } + + before do + tree_id = project.repository.tree.trees.first.id + project.repository.add_tag(user, tag_name, tree_id) + end + + it "does not create a pipeline", :sidekiq_inline do + expect { service.execute }.not_to change { Ci::Pipeline.count } + end end end -- GitLab