Merge branch 'smh-fix-ambig-tag' into 'master'

Delete tags by canonical reference name when mirroring repository

Closes #1439

See merge request gitlab-org/gitaly!2026

5 files changed, 29 insertions, 14 deletions

diff --git a/changelogs/unreleased/smh-fix-ambig-tag.yml b/changelogs/unreleased/smh-fix-ambig-tag.yml
new file mode 100644
index 000000000..a9dba39c0
--- /dev/null
+++ b/changelogs/unreleased/smh-fix-ambig-tag.yml
@@ -0,0 +1,5 @@
+---
+title: Delete tags by canonical reference name when mirroring repository
+merge_request: 2026
+author:
+type: fixed
diff --git a/internal/service/remote/update_remote_mirror_test.go b/internal/service/remote/update_remote_mirror_test.go
index 5242fc0df..885abe9e0 100644
--- a/internal/service/remote/update_remote_mirror_test.go
+++ b/internal/service/remote/update_remote_mirror_test.go
@@ -48,6 +48,7 @@ func TestSuccessfulUpdateRemoteMirrorRequest(t *testing.T) {
 		// on 'HEAD' as the default branch). By making HEAD point to something
 		// invalid, we ensure this gets handled correctly.
 		{"symbolic-ref", "HEAD", "refs/does/not/exist"},
+		{"tag", "--delete", "v1.1.0"}, // v1.1.0 is ambiguous, maps to a branch and a tag in gitlab-test repository
 	}
 
 	for _, args := range setupCommands {
@@ -93,6 +94,8 @@ func TestSuccessfulUpdateRemoteMirrorRequest(t *testing.T) {
 	require.Contains(t, mirrorRefs, "60ecb67744cb56576c30214ff52294f8ce2def98 commit\trefs/tags/new-tag")
 	require.Contains(t, mirrorRefs, newTagOid+" tag\trefs/tags/v1.0.0")
 	require.NotContains(t, mirrorRefs, "refs/tags/v0.0.1")
+	require.Contains(t, mirrorRefs, "refs/heads/v1.1.0")
+	require.NotContains(t, mirrorRefs, "refs/tags/v1.1.0")
 }
 
 func TestSuccessfulUpdateRemoteMirrorRequestWithWildcards(t *testing.T) {
@@ -123,6 +126,7 @@ func TestSuccessfulUpdateRemoteMirrorRequestWithWildcards(t *testing.T) {
 		{"update-ref", "refs/heads/feature", "0b4bc9a49b562e85de7cc9e834518ea6828729b9"},     // Update branch
 		// Scoped to the project, so will be removed after
 		{"branch", "-D", "not-merged-branch"}, // Delete branch
+		{"tag", "--delete", "v1.1.0"},         // v1.1.0 is ambiguous, maps to a branch and a tag in gitlab-test repository
 	}
 
 	testhelper.CreateTag(t, testRepoPath, "new-tag", "60ecb67744cb56576c30214ff52294f8ce2def98", nil) // Add tag
@@ -170,6 +174,8 @@ func TestSuccessfulUpdateRemoteMirrorRequestWithWildcards(t *testing.T) {
 	require.Contains(t, mirrorRefs, "60ecb67744cb56576c30214ff52294f8ce2def98 commit\trefs/tags/new-tag")
 	require.Contains(t, mirrorRefs, newTagOid+" tag\trefs/tags/v1.0.0")
 	require.NotContains(t, mirrorRefs, "refs/tags/v1.2.0")
+	require.Contains(t, mirrorRefs, "refs/heads/v1.1.0")
+	require.NotContains(t, mirrorRefs, "refs/tags/v1.1.0")
 }
 
 func TestSuccessfulUpdateRemoteMirrorRequestWithKeepDivergentRefs(t *testing.T) {
diff --git a/ruby/lib/gitlab/git/remote_mirror.rb b/ruby/lib/gitlab/git/remote_mirror.rb
index 02a35026d..8b5d5d4d3 100644
--- a/ruby/lib/gitlab/git/remote_mirror.rb
+++ b/ruby/lib/gitlab/git/remote_mirror.rb
@@ -59,7 +59,8 @@ module Gitlab
         refs.each_with_object({}) do |ref, refs|
           next if match_refs && !include_ref?(ref.name)
 
-          refs[ref.name] = ref
+          key = ref.is_a?(Gitlab::Git::Tag) ? ref.refname : ref.name
+          refs[key] = ref
         end
       end
 
diff --git a/ruby/lib/gitlab/git/repository_mirroring.rb b/ruby/lib/gitlab/git/repository_mirroring.rb
index 4b226fef9..ce6faec36 100644
--- a/ruby/lib/gitlab/git/repository_mirroring.rb
+++ b/ruby/lib/gitlab/git/repository_mirroring.rb
@@ -67,22 +67,21 @@ module Gitlab
         # Each line has this format: "dc872e9fa6963f8f03da6c8f6f264d0845d6b092\trefs/tags/v1.10.0\n"
         # We want to convert it to: [{ 'v1.10.0' => 'dc872e9fa6963f8f03da6c8f6f264d0845d6b092' }, ...]
         list_remote_tags(remote, env: env).map do |line|
-          target, path = line.strip.split("\t")
+          target, refname = line.strip.split("\t")
 
           # When the remote repo does not have tags.
-          if target.nil? || path.nil?
+          if target.nil? || refname.nil?
             Rails.logger.info "Empty or invalid list of tags for remote: #{remote}"
             break []
           end
 
-          name = path.split('/', 3).last
           # We're only interested in tag references
           # See: http://stackoverflow.com/questions/15472107/when-listing-git-ls-remote-why-theres-after-the-tag-name
-          next if name =~ /\^\{\}\Z/
+          next if refname.end_with?('^{}')
 
           target_commit = Gitlab::Git::Commit.find(self, target)
           Gitlab::Git::Tag.new(self,
-                               name: name,
+                               name: refname,
                                target: target,
                                target_commit: target_commit)
         end.compact
diff --git a/ruby/spec/lib/gitlab/git/remote_mirror_spec.rb b/ruby/spec/lib/gitlab/git/remote_mirror_spec.rb
index 2b7bc0605..e4611bce0 100644
--- a/ruby/spec/lib/gitlab/git/remote_mirror_spec.rb
+++ b/ruby/spec/lib/gitlab/git/remote_mirror_spec.rb
@@ -27,6 +27,10 @@ describe Gitlab::Git::RemoteMirror do
     double("ref-#{name}", name: name, dereferenced_target: double(id: name))
   end
 
+  def tag(name)
+    Gitlab::Git::Tag.new(nil, name: "refs/tags/#{name}", target_commit: double(id: name))
+  end
+
   describe '#update' do
     context 'with wildcard protected branches' do
       subject(:remote_mirror) do
@@ -48,10 +52,10 @@ describe Gitlab::Git::RemoteMirror do
           .with(ref_name)
           .and_return([ref('master'), ref('obsolete-branch')])
 
-        expect(repository).to receive(:tags).and_return([ref('v1.0.0'), ref('new-tag')])
+        expect(repository).to receive(:tags).and_return([tag('v1.0.0'), tag('new-tag')])
         expect(repository).to receive(:remote_tags)
           .with(ref_name, env: env)
-          .and_return([ref('v1.0.0'), ref('obsolete-tag')])
+          .and_return([tag('v1.0.0'), tag('obsolete-tag')])
 
         expect(gitlab_projects)
           .to receive(:push_branches)
@@ -60,7 +64,7 @@ describe Gitlab::Git::RemoteMirror do
 
         expect(gitlab_projects)
           .to receive(:push_branches)
-          .with(ref_name, gl_projects_timeout, gl_projects_force, ['v1.0.0', 'new-tag'], env: env)
+          .with(ref_name, gl_projects_timeout, gl_projects_force, ['refs/tags/v1.0.0', 'refs/tags/new-tag'], env: env)
           .and_return(true)
 
         # Leave remote branches that do not match the protected branch filter
@@ -70,7 +74,7 @@ describe Gitlab::Git::RemoteMirror do
 
         expect(gitlab_projects)
           .to receive(:delete_remote_branches)
-          .with(ref_name, ['obsolete-tag'], env: env)
+          .with(ref_name, ['refs/tags/obsolete-tag'], env: env)
           .and_return(true)
 
         remote_mirror.update
@@ -86,10 +90,10 @@ describe Gitlab::Git::RemoteMirror do
         .with(ref_name)
         .and_return([ref('master'), ref('obsolete-branch')])
 
-      expect(repository).to receive(:tags).and_return([ref('v1.0.0'), ref('new-tag')])
+      expect(repository).to receive(:tags).and_return([tag('v1.0.0'), tag('new-tag')])
       expect(repository).to receive(:remote_tags)
         .with(ref_name, env: env)
-        .and_return([ref('v1.0.0'), ref('obsolete-tag')])
+        .and_return([tag('v1.0.0'), tag('obsolete-tag')])
 
       expect(gitlab_projects)
         .to receive(:push_branches)
@@ -98,7 +102,7 @@ describe Gitlab::Git::RemoteMirror do
 
       expect(gitlab_projects)
         .to receive(:push_branches)
-        .with(ref_name, gl_projects_timeout, gl_projects_force, ['v1.0.0', 'new-tag'], env: env)
+        .with(ref_name, gl_projects_timeout, gl_projects_force, ['refs/tags/v1.0.0', 'refs/tags/new-tag'], env: env)
         .and_return(true)
 
       expect(gitlab_projects)
@@ -108,7 +112,7 @@ describe Gitlab::Git::RemoteMirror do
 
       expect(gitlab_projects)
         .to receive(:delete_remote_branches)
-        .with(ref_name, ['obsolete-tag'], env: env)
+        .with(ref_name, ['refs/tags/obsolete-tag'], env: env)
         .and_return(true)
 
       remote_mirror.update