From: Karthik Nayak <karthik.188@gmail.com>
Date: Wed, 17 Sep 2025 07:39:31 GMT
Subject: Re: [PATCH v3 2/4] refs/files: use correct error type when lock exists
Message-ID: <CAOLa=ZSy06tp7PbTOq+L77U4YqA6G1OEZbPb4F6S2dUw4RkqUQ@mail.gmail.com>
In-Reply-To: <672jdldtxegx4y3gdtcyrnsz5nsy4sh6pk76eftqrukwug3f3c@xwso5f4vukn5>

Justin Tobler <jltobler@gmail.com> writes:

> On 25/09/13 10:54PM, Karthik Nayak wrote:
>> When fetching references into a repository, if a lock for a particular
>> reference exists, then `lock_raw_ref()` throws:
>>
>>     - REF_TRANSACTION_ERROR_CASE_CONFLICT: when there is a conflict
>>     because transaction contains conflicting references while being on a
>
> s/transaction/the transaction/
>

Makes sense.

>>     case-insensitive filesystem.
>>
>>     - REF_TRANSACTION_ERROR_GENERIC: for all other errors.
>>
>> The latter causes the entire set of batched updates to fail, even in
>> case sensitive filessystems.
>
> Ok so this issue isn't related to case-insensitive filesystems. The
> issue is that now we use batch updated, a single pre-existing lockfile
> causes the entire transaction to fail. Prior to batch updates, only the
> individual update would fail, but wouldn't stop others.
>
>> Instead, return a 'REF_TRANSACTION_ERROR_CREATE_EXISTS' error. This
>> allows batched updates to reject the individual update which conflicts
>> with the existing file, while updating the rest of the references.
>
> Make sense.
>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>>  refs/files-backend.c | 20 +++++++++++++++++---
>>  t/t5510-fetch.sh     | 26 ++++++++++++++++++++++++++
>>  2 files changed, 43 insertions(+), 3 deletions(-)
>>
>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index 01df32904b..69e50a16db 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -797,9 +797,23 @@ static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs,
>>  			goto retry;
>>  		} else {
>>  			unable_to_lock_message(ref_file.buf, myerr, err);
>> -			if (myerr == EEXIST && ignore_case &&
>> -			    transaction_has_case_conflicting_update(transaction, update))
>> -				ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
>> +			if (myerr == EEXIST) {
>> +				if (ignore_case &&
>> +				    transaction_has_case_conflicting_update(transaction, update))
>> +					/*
>> +					 * In case-insensitive filesystems, ensure that conflicts within a
>> +					 * given transaction are handled. Pre-existing refs on a
>> +					 * case-insensitive system will be overridden without any issue.
>> +					 */
>> +					ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
>> +				else
>> +					/*
>> +					 * Pre-existing case-conflicting reference locks should also be
>> +					 * specially categorized to avoid failing all batched updates.
>> +					 */
>> +					ret = REF_TRANSACTION_ERROR_CREATE_EXISTS;
>
> IIUC, by returning a non-generic error here the individual reference
> will be rejected during batch updates instead of aborting the
> transaction.
>
> -Justin

Indeed. Batched updates allow rejecting of individual updates. It only
aborts when faced with a GENERIC error. So by marking this as
non-generic, it doesn't change the flow for regular transactions but
only how batched updates would handle it.