Patrick Steinhardt <ps@xxxxxx> writes:
> On Thu, Jun 05, 2025 at 10:19:55AM +0200, Karthik Nayak 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. A
>> similar issue was present in 'git-fetch(1)' and was fixed by using
>> separating out reference pruning into a separate transaction. 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 exactly two transactions,
>> whereas before using batched updates it would use _at least_ two
>> transactions. So using batched updates is the still the better option.
>
> s/the still the/still the/
>
Will fix.
>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
>> index 9e3cfb85cf..34db4377ca 100644
>> --- a/builtin/receive-pack.c
>> +++ b/builtin/receive-pack.c
>> @@ -1867,47 +1867,66 @@ 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
>> + * 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 (int phase = PHASE_DELETIONS; phase <= PHASE_OTHERS; phase++) {
>
> s/int/enum processing_phase/
>
> Doesn't make any difference, but it feels a bit cleaner.
>
Yeah, agreed.
>> + 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";
>> + goto failure;
>> + }
>
> So if the transaction doesn't contain any deletions we'd now commit an
> empty transaction. The same is true the other way round, in case there
> are only deletions. Do we maybe want to skip phases when there is no
> match? Ideally, we wouldn't even be starting a transaction.
>
> We could for example skip forward to the first command that we would
> have to queue. If there is no such command we continue the loop, if
> there is we can remember that command, begin the transaction and start
> queueing from there.
>
I think that's a fair point, and very simple to implement. I'll add that
in. Thanks.
>> @@ -2024,6 +2043,9 @@ static void execute_commands(struct command *commands,
>> /*
>> * If there is no command ready to run, should return directly to destroy
>> * temporary data in the quarantine area.
>> + *
>> + * Check if any reference deletions exist, these are batched together in
>> + * a separate transaction to avoid F/D conflicts with other updates.
>> */
>
> Is this comment still accurate?
>
Nope, will remove this.
>> for (cmd = commands; cmd && cmd->error_string; cmd = cmd->next)
>> ; /* nothing */
>> diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
>> index d91dd3a3b5..b2aaa1908f 100755
>> --- a/t/t1416-ref-transaction-hooks.sh
>> +++ b/t/t1416-ref-transaction-hooks.sh
>> @@ -119,6 +119,8 @@ test_expect_success 'interleaving hook calls succeed' '
>> EOF
>>
>> cat >expect <<-EOF &&
>> + hooks/reference-transaction prepared
>> + hooks/reference-transaction committed
>
> Yeah, this shows the empty commits indeed.
>
Yup, thanks for the quick review.
> Patrick
Attachment:
signature.asc
Description: PGP signature
