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@xxxxxxxxx>
> ---
> 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
