Christian Couder <christian.couder@xxxxxxxxx> writes:
> On Fri, Jun 6, 2025 at 10:41 AM Karthik Nayak <karthik.188@xxxxxxxxx> 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/
>
That's better, will change.
>> 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@xxxxxxxxx>
>> ---
>> 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.
>
Will change.
>> + * 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/
>
Oops! Good catch.
>> + 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 */
>
Looks better.
>> + 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.
>
Yup, but now this label lies within a for loop, so the indentation is
aligned to the loop. So I think it is correct as is.
>> + 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.
>
Same here, this too is within a loop.
>> + ref_transaction_free(transaction);
>> + transaction = NULL;
>> + strmap_clear(&failed_refs, 0);
>> + strbuf_release(&err);
>> + }
>> }
Thanks for the review!
- Karthik
Attachment:
signature.asc
Description: PGP signature
