Re: [PATCH v3 2/2] receive-pack: handle reference deletions separately
- From
Christian Couder <christian.couder@gmail.com>
- Date
- Jun 12, 2025, 17:03 UTC
- Message-ID
- <CAP8UFD0Az9YUd7tHbCWjrZ5bTv1V_0RZ2azasPmOrpf+ARMjug@mail.gmail.com>
- In-Reply-To
- <20250606-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v3-2-e1c41693bd35@gmail.com>
On Fri, Jun 6, 2025 at 10:41 AM Karthik Nayak <karthik.188@gmail.com> wrote:
Show 12 quoted lines
> > 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/
Show 33 quoted lines
> 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 toNit: here also maybe "D/F conflicts" is more standard.
Show 41 quoted lines
> + * 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/
Show 15 quoted lines
> + 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 */
Show 28 quoted lines
> + 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.
Show 8 quoted lines
> + 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.
Show 6 quoted lines
> + ref_transaction_free(transaction); > + transaction = NULL; > + strmap_clear(&failed_refs, 0); > + strbuf_release(&err); > + } > }