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

[Patrick Steinhardt <ps@pks.im>]

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?

Patrick