Patrick Steinhardt [off-list ref] writes:

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

quoted

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?

We don't hit this edge case in any of our tests I presume. This
basically requires a deletion request with an expected old OID, however
the reference should be non-existent.

quoted

Signed-off-by: Karthik Nayak <redacted>
---
 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.

Exactly, all writes in the reftable backend are propagated finally via
'write_transaction_table()', which checks for rejections. The single
point of entry acts as a good gatekeeper.

quoted

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

Will change, Thanks!