Re: [PATCH v2 1/4] refs/files: catch conflicts on case-insensitive file-systems

From: Karthik Nayak <karthik.188@gmail.com>
Date: Sep 11, 2025, 09:35 UTC
Message-ID: <CAOLa=ZRDY6ORDngq0Np0Y_8SFXRWWKAmmsecQfpqeqrLCfbN0g@mail.gmail.com>
In-Reply-To: <aL_SwghVaAXL-yeX@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 15 quoted lines

> On Mon, Sep 08, 2025 at 02:37:35PM +0200, Karthik Nayak wrote:
>> diff --git a/refs.h b/refs.h
>> index eedbb599c5..41915086b3 100644
>> --- a/refs.h
>> +++ b/refs.h
>> @@ -31,6 +31,8 @@ enum ref_transaction_error {
>>  	REF_TRANSACTION_ERROR_INVALID_NEW_VALUE = -6,
>>  	/* Expected ref to be symref, but is a regular ref */
>>  	REF_TRANSACTION_ERROR_EXPECTED_SYMREF = -7,
>> +	/* Cannot create ref due to case-insensitive filesystem */
>> +	REF_TRANSACTION_ERROR_CASE_CONFLICT = -8,
>
> Nice that we now have a specific error code for this error case. It
> removes some of the guesswork we previously had to do.
>

Yeah Agreed. I dislike adding a common error which only affects a single reference backend, but this is nicer.

Show 14 quoted lines

>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index 088b52c740..58005d2732 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -647,6 +647,19 @@ static void unlock_ref(struct ref_lock *lock)
>>  	}
>>  }
>>
>> +static bool duplicate_reference_case_cmp(struct ref_transaction *transaction,
>> +					 struct ref_update *update)
>
> I think the name could use some improvement. How about
> `transaction_has_case_conflicting_update()`?
>

That does read better, will change.

Show 7 quoted lines

>> +{
>> +	for (size_t i = 0; i < transaction->nr; i++) {
>> +		if (transaction->updates[i] == update)
>> +			break;
>
> Why do we break here? Shouldn't we continue?
>

We break because we only care about matching updates up to the index of the provided updates. Further updates should recall the function.

I'll add a comment to explain this.

Show 16 quoted lines

>> @@ -776,6 +790,9 @@ 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;
>>  			goto error_return;
>>  		}
>>  	}
>
> Okay. If we cannot lock the reference we now try to detect whether this
> is because of a case conflict. That only catches the case though where
> we have a case conflict in the same transaction, right? How about the
> case where there's preexisting refs on disk that cause a conflict?
>

Existing references aren't an issue since in those situations we can create the lock file. The issue here arises because within the same transaction we try to create the lock file twice which causes the conflict.

> Patrick