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

From: Karthik Nayak <karthik.188@gmail.com>
Date: Jun 2, 2025, 12:46 UTC
Message-ID: <CAOLa=ZQQCFA=jdOGDONGcsO_VDUAxfHjn_Wk5r+8weHMFXDQ3w@mail.gmail.com>
In-Reply-To: <aD2SRmlSKZm8g8kn@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 16 quoted lines

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

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.

Show 28 quoted lines

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

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.

Show 24 quoted lines

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