From f821abd5a85fdb23ef1e3b5e12af796c119cf2a2 Mon Sep 17 00:00:00 2001
From: Mathieu Leplatre <mathieu@mozilla.com>
Date: Mon, 15 Jun 2026 10:25:18 +0200
Subject: [PATCH] RMST-431: split push and delete tags

---
 .../src/commands/_git_export_git_tools.py     | 26 +++++--
 .../commands/test_git_export_git_tools.py     | 70 +++++++++++++++++++
 uv.lock                                       |  2 +-
 3 files changed, 90 insertions(+), 8 deletions(-)

diff --git a/cronjobs/src/commands/_git_export_git_tools.py b/cronjobs/src/commands/_git_export_git_tools.py
index b979f112..459b4a69 100644
--- a/cronjobs/src/commands/_git_export_git_tools.py
+++ b/cronjobs/src/commands/_git_export_git_tools.py
@@ -108,25 +108,37 @@ def push_mirror(
     # 1) Force update all local branches
     for b in sorted(branches):
         to_push.append(f"+{b}:{b}")
-    # 2) Force update all local tags and delete old tags
+    # 2) Force update all local tags
+    to_delete = []
     for t in sorted(tags):
         deleting = t.startswith("-")
         name = t[1:]
         full = name if name.startswith("refs/tags/") else f"refs/tags/{name}"
         if deleting:
-            to_push.append(f":{full}")
+            to_delete.append(f":{full}")
         else:
             to_push.append(f"+{full}:{full}")
 
-    if not to_push:
+    if not to_push and not to_delete:
         print("Everything up-to-date.")
         return
 
+    # First push the new content, and then delete old tags in a second push.
+    # We do that because if the remote times out or fails with tags deletion,
+    # at least the new content is pushed.
     remote = repo.remotes[REMOTE_NAME]
-    print(f"Pushing to remote {remote.url}:\n - {'\n - '.join(to_push)}")
-    # This is the critical bit: non-fast-forward updates require the '+' force.
-    # The deletions use the ':refs/...'; '+' is ignored for deleted refspecs.
-    remote.push(to_push, callbacks=callbacks)
+    if to_push:
+        print(f"Pushing to remote {remote.url}:\n - {'\n - '.join(to_push)}")
+        # This is the critical bit: non-fast-forward updates require the '+' force.
+        # The deletions use the ':refs/...'; '+' is ignored for deleted refspecs.
+        remote.push(to_push, callbacks=callbacks)
+    else:
+        print("No new commit or tag to push.")
+    if to_delete:
+        print(f"Deleting from remote {remote.url}:\n - {'\n - '.join(to_delete)}")
+        remote.push(to_delete, callbacks=callbacks)
+    else:
+        print("No tag to delete.")
 
 
 def make_lfs_pointer(sha256_hex: str, size: int) -> bytes:
diff --git a/cronjobs/tests/commands/test_git_export_git_tools.py b/cronjobs/tests/commands/test_git_export_git_tools.py
index e5c16f0f..89ec8c43 100644
--- a/cronjobs/tests/commands/test_git_export_git_tools.py
+++ b/cronjobs/tests/commands/test_git_export_git_tools.py
@@ -8,6 +8,7 @@
     delete_old_tags,
     iter_tree,
     parse_lfs_pointer,
+    push_mirror,
     reset_repo,
     tree_upsert_blobs,
     truncate_branch,
@@ -47,6 +48,12 @@ def mock_ls_remotes():
         yield mock_ls
 
 
+@pytest.fixture
+def mock_remote_push():
+    with mock.patch("pygit2.Remote.push") as mock_push:
+        yield mock_push
+
+
 def test_valid_lfs_pointer():
     data = b"""version https://git-lfs.github.com/spec/v1
 oid sha256:1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef
@@ -100,6 +107,69 @@ def test_reset_repo_creates_local_branches(tmp_repo, mock_ls_remotes):
     assert local_ref.target == remote_ref.target
 
 
+@pytest.mark.parametrize(
+    ("branches", "tags", "expected"),
+    [
+        (
+            [
+                "v1/common",
+            ],
+            [
+                "-refs/tags/timestamp/123",
+                "+refs/tags/timestamp/456",
+            ],
+            [
+                [
+                    "+v1/common:v1/common",
+                    "+refs/tags/timestamp/456:refs/tags/timestamp/456",
+                ],
+                [":refs/tags/timestamp/123"],
+            ],
+        ),
+        (
+            [],
+            [
+                # Test that tags are normalized.
+                "+timestamp/789",
+            ],
+            [
+                [
+                    "+refs/tags/timestamp/789:refs/tags/timestamp/789",
+                ]
+                # And no call for delete.
+            ],
+        ),
+        (
+            [],
+            [
+                "-refs/tags/timestamp/123",
+            ],
+            [
+                [
+                    ":refs/tags/timestamp/123",
+                ]
+                # Only 1 call for delete.
+            ],
+        ),
+    ],
+)
+def test_push_mirror_pushes_branches_and_tags_then_deletes(
+    tmp_repo, mock_remote_push, branches, tags, expected
+):
+    repo = tmp_repo
+
+    push_mirror(
+        repo,
+        branches,
+        tags,
+        callbacks=None,
+    )
+
+    assert mock_remote_push.call_count == len(expected)
+    for expect in expected:
+        mock_remote_push.assert_any_call(expect, callbacks=None)
+
+
 def test_reset_repo_resets_local_branches_to_remote(tmp_repo, mock_ls_remotes):
     repo = tmp_repo
     commit = repo.revparse_single("main")
diff --git a/uv.lock b/uv.lock
index 9a6af000..140d8f5f 100644
--- a/uv.lock
+++ b/uv.lock
@@ -629,7 +629,7 @@ requires-dist = [
     { name = "asgiref", specifier = ">=3.11.0" },
     { name = "dockerflow", specifier = ">=2026.3.4" },
     { name = "fastapi", specifier = ">=0.120.2" },
-    { name = "granian", specifier = ">=2.7.5" },
+    { name = "granian", specifier = ">=2.7.6" },
     { name = "lz4", specifier = ">=4.4.5" },
     { name = "prometheus-client", specifier = ">=0.23.1" },
     { name = "pydantic-settings", specifier = ">=2.10.1" },