Re: [PATCH 2/2] refs/files: handle F/D conflicts in case-insensitive FS

> On Tue, Sep 02, 2025 at 10:34:26AM +0200, Karthik Nayak wrote:
>> Similar to the previous commit, when using the files-backend on
>> case-insensitive filesystems, there is possibility of hitting F/D
>> conflicts when creating references within a single transaction, such as:
>>
>>   - 'refs/heads/foo'
>>   - 'refs/heads/Foo/bar'
>
> Great, I wanted to ask about this scenario.
>
>> Ideally such conflicts are caught in `refs_verify_refnames_available()`
>> which is responsible for checking F/D conflicts within a given
>> transaction. This utility function is shared across the reference
>> backends. As such, it doesn't consider the issues of using a
>> case-insensitive, which only affects the files-backend.
>>
>> While one solution would be to make the function aware of such issues.
>> This feels like leaking implementation details of file-backend specific
>> issues into the utility function. So opt for the more simpler option, of
>> lowercasing all references sent to this function when on a
>> case-insensitive filesystem and operating on the files-backend.
>>
>> To do this, simply use a `struct strbuf` to convert the refname to a
>> lower case and append it to the list of refnames to be checked. Since we
>> use a `struct strbuf` and the memory is cleared right after, make sure
>> that the string list duplicates all provided string.
>>
>> Without this change, the user would simply be left with a repository
>> with '.lock' files which were created in the 'prepare' phase of the
>> transaction, as the 'commit' phase would simply abort and not do the
>> necessary cleanup.
>
> Oh, that's a clever hack. Does this also work for the case where we have
> preexisting refs already that differ only in casing? I guess it should
> given that the lookups we perform should yield those refs regardless of
> their casing.
>
> In any case, if we don't already have such a test it would be great to
> also verify that this works as expected.
>

>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index 9f58ea4858..466cdfe121 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -869,8 +869,23 @@ static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs,
>>  		 * If the ref did not exist and we are creating it, we have to
>>  		 * make sure there is no existing packed ref that conflicts
>>  		 * with refname. This check is deferred so that we can batch it.
>> +		 *
>> +		 * For case-insensitive filesystems, we should also check for F/D
>> +		 * conflicts between 'foo' and 'Foo/bar'. So let's lowercase
>> +		 * the refname.
>>  		 */
>> -		item = string_list_append(refnames_to_check, refname);
>> +		if (ignore_case) {
>> +			struct strbuf lower = STRBUF_INIT;
>> +
>> +			strbuf_addstr(&lower, refname);
>> +			strbuf_tolower(&lower);
>> +
>> +			item = string_list_append(refnames_to_check, lower.buf);
>> +			strbuf_release(&lower);
>
> Can we use `string_list_append_nodup()` together with `strbuf_detach()`
> here to avoid one memory allocation?
>

>> +		} else {
>> +			item = string_list_append(refnames_to_check, refname);
>> +		}
>> +
>>  		item->util = xmalloc(sizeof(update_idx));
>>  		memcpy(item->util, &update_idx, sizeof(update_idx));
>>  	}
>> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
>> index 57f60da81b..84dc68e5f3 100755
>> --- a/t/t5510-fetch.sh
>> +++ b/t/t5510-fetch.sh
>> @@ -53,6 +53,12 @@ test_expect_success "clone and setup child repos" '
>>  		cd case_sensitive &&
>>  		git branch branch1 &&
>>  		git branch bRanch1
>> +	) &&
>> +	git clone --ref-format=reftable . case_sensitive_fd &&
>> +	(
>> +		cd case_sensitive_fd &&
>> +		git branch foo/bar &&
>> +		git branch Foo
>>  	)
>>  '
>>
>
> Nice idea to use the reftable format here.
>

>> @@ -1546,6 +1552,20 @@ test_expect_success CASE_INSENSITIVE_FS,REFFILES 'existing references in a case
>>  	)
>>  '
>>
>> +test_expect_success CASE_INSENSITIVE_FS,REFFILES 'F/D conflict on case insensitive filesystem' '
>> +	test_when_finished rm -rf case_insensitive &&
>> +	(
>> +		git init --bare case_insensitive &&
>> +		cd case_insensitive &&
>> +		git remote add origin -- ../case_sensitive_fd &&
>> +		test_must_fail git fetch -f origin "refs/heads/*:refs/heads/*" 2>err &&
>> +		test_grep "failed: refname conflict" err &&
>> +		git rev-parse refs/heads/main >expect &&
>> +		git rev-parse refs/heads/foo/bar >actual &&
>> +		test_cmp expect actual
>> +	)
>> +'
>
> Okay, so we again only end up with one of these references, which is the
> best we can do.
>