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

[Patrick Steinhardt <ps@pks.im>]

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.

Patrick