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

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

>> 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.
>

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

>> @@ -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?
>

>>  	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.
>