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

[Christian Couder <christian.couder@gmail.com>]

On Fri, Jun 6, 2025 at 10:41 AM Karthik Nayak <karthik.188@gmail.com> 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/

> 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 upto 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>
> ---
>  builtin/receive-pack.c | 100 ++++++++++++++++++++++++++++++++-----------------
>  t/t5516-fetch-push.sh  |  17 +++++++--
>  2 files changed, 79 insertions(+), 38 deletions(-)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 9e3cfb85cf..8ee792d2f8 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1867,47 +1867,79 @@ 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 F/D conflicts shouldn't arise due to

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);
> +       }
>  }