From: Karthik Nayak Date: Mon, 08 Sep 2025 07:27:03 GMT Subject: Re: [PATCH 2/2] refs/files: handle F/D conflicts in case-insensitive FS Message-ID: In-Reply-To: Patrick Steinhardt writes: > 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. > There is a case which I missed and Junio also highlighted, I will fix that in the consequent version. >> 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? > That's clever, I'll add that in, thanks! >> + } 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. > Yeah, that was the only way. >> @@ -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. > Exactly. > atrick