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

> On Mon, Jun 02, 2025 at 11:57:26AM +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.
>>
>> Add a test to validate this behavior.
>
> Okay. All of this is unfortunate as ideally the reference transaction
> itself would know to resolve such conflicts. But we're no worse off than
> before because we at most perform exactly two transactions now, whereas
> before we would have performed _at least_ two transactions in this
> conflicting case.
>

>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>>  builtin/receive-pack.c           | 23 +++++++++++++++++++----
>>  t/t1416-ref-transaction-hooks.sh |  2 ++
>>  t/t5516-fetch-push.sh            | 17 +++++++++++++----
>>  3 files changed, 34 insertions(+), 8 deletions(-)
>>
>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
>> index 9e3cfb85cf..7157ced2a6 100644
>> --- a/builtin/receive-pack.c
>> +++ b/builtin/receive-pack.c
>> @@ -1879,6 +1880,8 @@ static void execute_commands_non_atomic(struct command *commands,
>>  	for (cmd = commands; cmd; cmd = cmd->next) {
>>  		if (!should_process_cmd(cmd) || cmd->run_proc_receive)
>>  			continue;
>> +		if (only_deletions ^ is_null_oid(&cmd->new_oid))
>> +			continue;
>>
>>  		cmd->error_string = update(cmd, si);
>>  	}
>
> Fancy.
>
>> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
>> index 029ef92d58..34eb3a5a07 100755
>> --- a/t/t5516-fetch-push.sh
>> +++ b/t/t5516-fetch-push.sh
>> @@ -744,8 +744,8 @@ test_expect_success 'pushing valid refs triggers post-receive and post-update ho
>>  		EOF
>>
>>  		cat >update.expect <<-EOF &&
>> -		refs/heads/main $orgmain $newmain
>>  		refs/heads/next $orgnext $newnext
>> +		refs/heads/main $orgmain $newmain
>>  		EOF
>>
>>  		cat >post-receive.expect <<-EOF &&
>
> Hm, so the ordering does change now as all deletes will now be listed
> before the updates. We don't make any guarantees about how these are
> sorted, but it makes me a bit uneasy to see this change. Can we avoid
> this change in behaviour somehow?
>