Re: [PATCH v2 2/4] refs/files: use correct error type when lock exists

From: Karthik Nayak <karthik.188@gmail.com>
Date: Sep 11, 2025, 10:14 UTC
Message-ID: <CAOLa=ZTf=Es5MJvdC=zTUWoJnASF=Mqf9+RXrnHOYDZ9YuFEtg@mail.gmail.com>
In-Reply-To: <aL_SydrNjXdQrtFC@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 9 quoted lines

> On Mon, Sep 08, 2025 at 02:37:36PM +0200, Karthik Nayak wrote:
>> When fetching references into a repository, if a lock for a particular
>> reference exists, then `lock_raw_ref()` throws the generic error
>> 'REF_TRANSACTION_ERROR_GENERIC'. This causes the entire set of batched
>> updates to fail.
>
> This isn't quite accurate anymore as we may also raise
> `REF_TRANSACTION_ERROR_CASE_CONFLICT` now.
>

Good catch, will fixup.

Show 27 quoted lines

>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index 58005d2732..2730713d23 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -790,9 +790,13 @@ 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 &&
>> -			    duplicate_reference_case_cmp(transaction, update))
>> -				ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
>> +			if (myerr == EEXIST) {
>> +				if (ignore_case && duplicate_reference_case_cmp(transaction, update))
>> +					ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
>> +				else
>> +					ret = REF_TRANSACTION_ERROR_CREATE_EXISTS;
>> +			}
>> +
>>  			goto error_return;
>>  		}
>>  	}
>
> Hm. So if I understand correctly, we now return CREATE_EXISTS in case we
> have a conflict with a preexisting case-conflicting reference, but we
> return CASE_CONFLICT in case we have a conflict with an update in the
> same transaction?
>

It's not a reference, rather the lock that causes the conflict. So a preexisting case-conflicting reference lock would raise the GENERIC error. Which would fail all updates.

Show 9 quoted lines

> It feels awkward, but I guess that's the best thing we can do. We
> happily overwrite case-conflicting preexisting refs, so we wouldn't even
> see EEXIST in that case. So the only case where we still see that error
> is on a D/F conflict, and in that case it makes sense to return
> CREATE_EXISTS.
>
> I feel like this should be added as a comment though, as it gives some
> important non-obvious context.
>

Yeah, it would be beneficial, will add some comments here.

> Patrick