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

From: Patrick Steinhardt <ps@pks.im>
Date: Jun 2, 2025, 11:59 UTC
Message-ID: <aD2SPsro694yr60Z@pks.im>
In-Reply-To: <20250602-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v1-3-903d1db3f10e@gmail.com>

On Mon, Jun 02, 2025 at 11:57:26AM +0200, Karthik Nayak wrote:

Show 17 quoted lines

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

Show 20 quoted lines

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

Show 14 quoted lines

> 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