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

> On 25/09/13 10:54PM, Karthik Nayak wrote:
>> 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'
>>
>> 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 file system, 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
>
> s/a lower case/lowercase/
>

>> 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.
>
> So IIUC, this also isn't related to the batched updates change and is
> just an existing issue caused by case-insensitive filesystems and F/D
> conflicts. With this change, we now properly detect F/D conflicts in
> these situations and thus are able to cleanup lockfiles that would
> previously be left behind.
>

>> Reported-by: Junio C Hamano <gitster@pobox.com>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>>  refs/files-backend.c | 19 +++++++++++++++++--
>>  t/t5510-fetch.sh     | 20 ++++++++++++++++++++
>>  2 files changed, 37 insertions(+), 2 deletions(-)
>>
>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index 69e50a16db..817b56f4ce 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -905,8 +905,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_nodup(refnames_to_check,
>> +							strbuf_detach(&lower, NULL));
>
> For case-insensitive file-systems, we instead append a lowercased
> version of the reference name which gets used to check for F/D
> conflicts. Makes sense.
>
>> +		} else {
>> +			item = string_list_append(refnames_to_check, refname);
>> +		}
>> +
>
> -Justin