Re: [PATCH 2/2] refs/files: handle F/D conflicts in case-insensitive FS
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 3, 2025, 18:16 UTC
- Message-ID
- <xmqqms7bfll4.fsf@gitster.g>
- In-Reply-To
- <20250902-587-git-fetch-1-fails-fetches-on-case-insensitive-repositories-v1-2-35e69bbb507d@gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
Show 12 quoted lines
> 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' > > 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.
Sounds like a sensible way to separate the issues and responsibilities between higher and lower layers.
> While one solution would be to make the function aware of such issues. > This feels like...
The first line alone is only half a sentence. "such issues. This" -> "such issues, this".
> ... leaking implementation details of file-backend specific > issues into the utility function.
Very true.
> 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.
So when you are trying to lock "Foo", you lock "foo", for example? How would that let the generic code liks verify_refname_available notice that an existing ref "Foo/bar" would crash with the name you are trying to take, which is now downcased to be "foo"? I am not sure if the above explanation is sufficiently clear to convince readers why it is sufficient..
> Reported-by: Junio C Hamano <gitster@pobox.com>
Hmph, I do not recall reporting anything, but perhaps it was a long time ago...
Show 81 quoted lines
> 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 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);
> + } else {
> + item = string_list_append(refnames_to_check, refname);
> + }
> +
> item->util = xmalloc(sizeof(update_idx));
> memcpy(item->util, &update_idx, sizeof(update_idx));
> }
> @@ -2796,7 +2811,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,
> "ref_transaction_prepare");
> size_t i;
> int ret = 0;
> - struct string_list refnames_to_check = STRING_LIST_INIT_NODUP;
> + struct string_list refnames_to_check = STRING_LIST_INIT_DUP;
> char *head_ref = NULL;
> int head_type;
> struct files_transaction_backend_data *backend_data;
> 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
> )
> '
>
> @@ -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
> + )
> +'
> +
> . "$TEST_DIRECTORY"/lib-httpd.sh
> start_httpd