Re: [PATCH 1/3] refs/files: skip updates with errors in batched updates

[Patrick Steinhardt <ps@pks.im>]

On Mon, Jun 02, 2025 at 11:57:24AM +0200, Karthik Nayak wrote:
> The commit 23fc8e4f61 (refs: implement batch reference update support,
> 2025-04-08) introduced support for batched reference updates. This
> allows users to batch updates together, while allowing some of the
> updates to fail.
> 
> Under the hood, batched updates use the reference transaction mechanism.
> Each update which fails is marked as such. Any failed updates must be
> skipped over in the rest of the code, as they wouldn't apply any more.
> In two of the loops within 'files_transaction_finish()' of the files
> backend, the failed updates aren't skipped over. This can cause a
> SEGFAULT otherwise. Add the missing skips and a test to validate the
> same.

Curious -- we do have tests, so why don't any of them hit this issue?

> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  refs/files-backend.c  |  7 +++++++
>  t/t1400-update-ref.sh | 14 ++++++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 4d1f65a57a..c4a0f29072 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -3208,6 +3208,10 @@ static int files_transaction_finish(struct ref_store *ref_store,
>  	 */
>  	for (i = 0; i < transaction->nr; i++) {
>  		struct ref_update *update = transaction->updates[i];
> +
> +		if (update->rejection_err)
> +			continue;
> +
>  		if (update->flags & REF_DELETING &&
>  		    !(update->flags & REF_LOG_ONLY) &&
>  		    !(update->flags & REF_IS_PRUNING)) {

Ok. And the reftable backend doesn't need the same treatment? Probably
doesn't because we queue all updates via `queue_transaction_update()`,
which knows to not add them to the list of updates in case any error has
happened. And in `_finish()` we iterate through that list of queued
updates instead of the global list of updates.

> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index d29d23cb89..e9a605d0ba 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -2299,6 +2299,20 @@ do
>  			test_grep -q "refname conflict" stdout
>  		)
>  	'
> +
> +	test_expect_success "stdin $type batch-updates delete non-existent ref" '
> +		git init repo &&
> +		test_when_finished "rm -fr repo" &&
> +		(
> +			cd repo &&
> +			test_commit commit &&
> +			head=$(git rev-parse HEAD) &&
> +
> +			format_command $type "delete refs/heads/non-existent" "$head" >stdin &&
> +			git update-ref $type --stdin --batch-updates <stdin >stdout &&
> +			test_grep -q "reference does not exist" stdout

We typically don't silence the output of `test_grep`.

Patrick