receive-pack: handle reference deletions separately

In 9d2962a7c4 (receive-pack: use batched reference updates, 2025-05-19)
we updated the 'git-receive-pack(1)' command to use batched reference
updates. One edge case which was missed during this implementation was
when a user pushes multiple branches such as:

  delete refs/heads/branch/conflict
  create refs/heads/branch

Before using batched updates, the references would be applied
sequentially and hence no conflicts would arise. With batched updates,
while the first update applies, the second fails due to D/F conflict. A
similar issue was present in 'git-fetch(1)' and was fixed by separating
out reference pruning into a separate transaction in the commit 'fetch:
use batched reference updates'. Apply a similar mechanism for
'git-receive-pack(1)' and separate out reference deletions into its own
batch.

This means 'git-receive-pack(1)' will now use up to two transactions,
whereas before using batched updates it would use _at least_ two
transactions. So using batched updates is still the better option.

Add a test to validate this behavior.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 9e3cfb85cf759e4231e1c893829d18c5f5f8f833..1809539201af0f60ba6d711dffb6305fcaba9fe6 100644 (file)
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1867,47 +1867,81 @@ static void execute_commands_non_atomic(struct command *commands,
        const char *reported_error = NULL;
        struct strmap failed_refs = STRMAP_INIT;
 
-       transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
-                                                 REF_TRANSACTION_ALLOW_FAILURE, &err);
-       if (!transaction) {
-               rp_error("%s", err.buf);
-               strbuf_reset(&err);
-               reported_error = "transaction failed to start";
-               goto failure;
-       }
+       /*
+        * Reference updates, where D/F conflicts shouldn't arise due to
+        * one reference being deleted, while the other being created
+        * are treated as conflicts in batched updates. This is because
+        * we don't do conflict resolution inside a transaction. To
+        * mitigate this, delete references in a separate batch.
+        *
+        * NEEDSWORK: Add conflict resolution between deletion and creation
+        * of reference updates within a transaction. With that, we can
+        * combine the two phases.
+        */
+       enum processing_phase {
+               PHASE_DELETIONS,
+               PHASE_OTHERS
+       };
 
-       for (cmd = commands; cmd; cmd = cmd->next) {
-               if (!should_process_cmd(cmd) || cmd->run_proc_receive)
-                       continue;
+       for (enum processing_phase phase = PHASE_DELETIONS; phase <= PHASE_OTHERS; phase++) {
+               for (cmd = commands; cmd; cmd = cmd->next) {
+                       if (!should_process_cmd(cmd) || cmd->run_proc_receive)
+                               continue;
 
-               cmd->error_string = update(cmd, si);
-       }
+                       if (phase == PHASE_DELETIONS && !is_null_oid(&cmd->new_oid))
+                               continue;
+                       else if (phase == PHASE_OTHERS && is_null_oid(&cmd->new_oid))
+                               continue;
 
-       if (ref_transaction_commit(transaction, &err)) {
-               rp_error("%s", err.buf);
-               reported_error = "failed to update refs";
-               goto failure;
-       }
+                       /*
+                        * Lazily create a transaction only when we know there are
+                        * updates to be added.
+                        */
+                       if (!transaction) {
+                               transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
+                                                                         REF_TRANSACTION_ALLOW_FAILURE, &err);
+                               if (!transaction) {
+                                       rp_error("%s", err.buf);
+                                       strbuf_reset(&err);
+                                       reported_error = "transaction failed to start";
+                                       goto failure;
+                               }
+                       }
 
-       ref_transaction_for_each_rejected_update(transaction,
-                                                ref_transaction_rejection_handler,
-                                                &failed_refs);
+                       cmd->error_string = update(cmd, si);
+               }
 
-       if (strmap_empty(&failed_refs))
-               goto cleanup;
+               /* No transaction, so nothing to commit */
+               if (!transaction)
+                       goto cleanup;
 
-failure:
-       for (cmd = commands; cmd; cmd = cmd->next) {
-               if (reported_error)
-                       cmd->error_string = reported_error;
-               else if (strmap_contains(&failed_refs, cmd->ref_name))
-                       cmd->error_string = strmap_get(&failed_refs, cmd->ref_name);
-       }
+               if (ref_transaction_commit(transaction, &err)) {
+                       rp_error("%s", err.buf);
+                       reported_error = "failed to update refs";
+                       goto failure;
+               }
 
-cleanup:
-       ref_transaction_free(transaction);
-       strmap_clear(&failed_refs, 0);
-       strbuf_release(&err);
+               ref_transaction_for_each_rejected_update(transaction,
+                                                        ref_transaction_rejection_handler,
+                                                        &failed_refs);
+
+               if (strmap_empty(&failed_refs))
+                       goto cleanup;
+
+       failure:
+               for (cmd = commands; cmd; cmd = cmd->next) {
+                       if (reported_error)
+                               cmd->error_string = reported_error;
+                       else if (strmap_contains(&failed_refs, cmd->ref_name))
+                               cmd->error_string = strmap_get(&failed_refs, cmd->ref_name);
+               }
+
+       cleanup:
+               ref_transaction_free(transaction);
+               transaction = NULL;
+               strmap_clear(&failed_refs, 0);
+               strbuf_release(&err);
+       }
 }
 
 static void execute_commands_atomic(struct command *commands,

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index dabcc5f8117645c8b74ada938e57c1a47d1a7ff3..1649667441fcb3fc985d3332f1efeb2eaf9dc80a 100755 (executable)
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -744,8 +744,8 @@ test_expect_success 'pushing valid refs triggers post-receive and post-update ho
                EOF
 
                cat >update.expect <<-EOF &&
-               refs/heads/main $orgmain $newmain
                refs/heads/next $orgnext $newnext
+               refs/heads/main $orgmain $newmain
                EOF
 
                cat >post-receive.expect <<-EOF &&
@@ -808,8 +808,8 @@ test_expect_success 'deletion of a non-existent ref is not fed to post-receive a
                EOF
 
                cat >update.expect <<-EOF &&
-               refs/heads/main $orgmain $newmain
                refs/heads/nonexistent $ZERO_OID $ZERO_OID
+               refs/heads/main $orgmain $newmain
                EOF
 
                cat >post-receive.expect <<-EOF &&
@@ -868,10 +868,10 @@ test_expect_success 'mixed ref updates, deletes, invalid deletes trigger hooks w
                EOF
 
                cat >update.expect <<-EOF &&
-               refs/heads/main $orgmain $newmain
                refs/heads/next $orgnext $newnext
-               refs/heads/seen $orgseen $newseen
                refs/heads/nonexistent $ZERO_OID $ZERO_OID
+               refs/heads/main $orgmain $newmain
+               refs/heads/seen $orgseen $newseen
                EOF
 
                cat >post-receive.expect <<-EOF &&
@@ -1909,4 +1909,13 @@ test_expect_success 'push with config push.useBitmaps' '
                --thin --delta-base-offset -q --no-use-bitmap-index <false
 '
 
+test_expect_success 'push with F/D conflict with deletion and creation' '
+       test_when_finished "git branch -D branch" &&
+       git branch branch/conflict &&
+       mk_test testrepo heads/branch/conflict &&
+       git branch -D branch/conflict &&
+       git branch branch &&
+       git push testrepo :refs/heads/branch/conflict refs/heads/branch
+'
+
 test_done