Re: [PATCH v3 2/2] receive-pack: handle reference deletions separately

On Fri, Jun 6, 2025 at 10:41 AM Karthik Nayak [off-list ref] wrote:

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 F/D conflict.

Nit: it looks like "D/F conflict" is more often used than "F/D
conflict" in the Git code base:

$ git grep -i 'd/f conflict' | wc -l
119
$ git grep -i 'f/d conflict' | wc -l
7

A
similar issue was present in 'git-fetch(1)' and was fixed by using
separating out reference pruning into a separate transaction. Apply a

Maybe: s/using separating out/separating out/

Nit: here also maybe "D/F conflicts" is more standard.

+        * 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.
+        */
+       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 s1tart";

s/s1tart/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;
+               /*
+                * If no transaction was created, there is nothing to commit.
+                */

Nit: the comment needs a single line, so maybe:

              /* 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:

This label looks indented while previously it was right at the start
of the line. Not sure if we have a standard for that, but a few quick
greps seems to show that goto labels are most often at the start of
the line.

+               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:

Idem for how this label is indented.

+               ref_transaction_free(transaction);
+               transaction = NULL;
+               strmap_clear(&failed_refs, 0);
+               strbuf_release(&err);
+       }
 }