[PATCH 0/2] refs/files: fix issues with git-fetch on case-insensitive FS

[PATCH 0/2] refs/files: fix issues with git-fetch on case-insensitive FS

[Karthik Nayak]

Hello!

With Git 2.51 we moved 'git-fetch(1)' and 'git-receive-pack(1)' to use
batched updates while doing reference updates. This provided a nice perf
boost since both commands will now use a single transaction for
reference updates. This removes the overhead of using individual
transaction per reference update and also avoids unnecessary
auto-compaction between reference updates in the reftable backend.

However, in the files-backend it does introduce two bugs when used on a
case-insensitive filesystem [1]:

1. When fetching references such as:

   - refs/heads/foo
   - refs/heads/Foo

Earlier we would simply overwrite the first reference with the second
and continue. Since Git 2.51 we simply abort stating a conflict.

This is resolved in the first commit by marking such conflicts as
REF_TRANSACTION_ERROR_CREATE_EXISTS. This allows batched updates to
reject the particular update, while applying the rest.

2. When fetching references such as:

  - refs/heads/foo
  - refs/heads/Foo/bar

Earlier we would apply the first, while the second would fail due to
conflict. Since Git 2.51, the lock files for both would be created, but
the 'commit' phase would abruptly end leaving the lock files.

The second commit fixes this by ensuring that on case-insensitive
filesystems we lowercase the refnames for availability check to ensure
F/D are caught and reported to the user.

Sorry for the long wait on this, with the OSSE and Git mini summit last
week, I didn't get as much computer time as I needed to test this.

- Karthik

[1]: https://lore.kernel.org/all/YQXPR01MB3046197EF39296549EE6DD669A33A@YQXPR01MB3046.CANPRD01.PROD.OUTLOOK.COM/

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/fetch.c       | 21 ++++++++++++++++++---
 refs/files-backend.c  | 21 +++++++++++++++++++--
 t/t1400-update-ref.sh | 24 ++++++++++++++++++++++++
 t/t5510-fetch.sh      | 42 +++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 102 insertions(+), 6 deletions(-)

Karthik Nayak (2):
      refs/files: use correct error type when locking fails
      refs/files: handle F/D conflicts in case-insensitive FS



base-commit: c44beea485f0f2feaf460e2ac87fdd5608d63cf0
change-id: 20250822-587-git-fetch-1-fails-fetches-on-case-insensitive-repositories-0a187c7ac41e

Thanks
- Karthik

[PATCH 1/2] refs/files: use correct error type when locking fails

[Karthik Nayak]

During the 'prepare' phase of reference transaction in the files
backend, we create the lock files for references to be created. When
using batched updates on case-insensitive filesystems, the transactions
would be aborted if there are conflicting names such as:

  refs/heads/Foo
  refs/heads/foo

This affects all commands which were migrated to use batched updates in
Git 2.51, including 'git-fetch(1)' and 'git-receive-pack(1)'. Before
that, references updates would be applied serially with one transaction
used per update. When users fetched multiple references on
case-insensitive systems, subsequent references would simply overwrite
any earlier references. So when fetching:

  refs/heads/foo: 5f34ec0bfeac225b1c854340257a65b106f70ea6
  refs/heads/Foo: ec3053b0977e83d9b67fc32c4527a117953994f3
  refs/heads/sample: 2eefd1150e06d8fca1ddfa684dec016f36bf4e56

The user would simply end up with:

  refs/heads/foo: ec3053b0977e83d9b67fc32c4527a117953994f3
  refs/heads/sample: 2eefd1150e06d8fca1ddfa684dec016f36bf4e56

This is buggy behavior since the user is never intimated about the
overrides performed and missing references. Nevertheless, the user is
left with a working repository with a subset of the references. Since
Git 2.51, in such situations fetches would simply fail without applying
any references. Which is also buggy behavior and worse off since the
user is left without any references.

The error is triggered in `lock_raw_ref()` where the files backend
attempts to create a lock file. When a lock file already exists the
function returns a 'REF_TRANSACTION_ERROR_GENERIC'. Change this to return
'REF_TRANSACTION_ERROR_CREATE_EXISTS' instead to aid the batched update
mechanism to simply reject such errors.

This bubbles the error type up to `files_transaction_prepare()` which
tries to lock each reference update. So if the locking fails, we check
if the rejection type can be ignored, which is done by calling
`ref_transaction_maybe_set_rejected()`.

As the error type is now 'REF_TRANSACTION_ERROR_CREATE_EXISTS', the
specific reference update would simply be rejected, while other updates
in the transaction would continue to be applied. This allows partial
application of references in case-insensitive filesystems when fetching
colliding references.

While the earlier implementation allowed the last reference to be
applied overriding the initial references, this change would allow the
first reference to be applied while rejecting consequent collisions.
This should be an OKAY compromise since with the files backend, there is
no scenario possible where we would retain all colliding references.

The change only affects batched updates since batched updates will
reject individual updates with non-generic errors. So specifically this
would only affect:

    1. git fetch
    2. git receive-pack
    3. git update-ref --batch-updates

Let's also be more pro-active and notify users on case-insensitive
filesystems about such problems by providing a brief about the issue
while also recommending using the reftable backend, which doesn't have
the same issue.

Reported-by: Joe Drew <joe.drew@indexexchange.com>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/fetch.c       | 21 ++++++++++++++++++---
 refs/files-backend.c  |  2 ++
 t/t1400-update-ref.sh | 24 ++++++++++++++++++++++++
 t/t5510-fetch.sh      | 22 +++++++++++++++++++++-
 4 files changed, 65 insertions(+), 4 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 24645c4653..9563abbe12 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1643,7 +1643,8 @@ static int set_head(const struct ref *remote_refs, struct remote *remote)
 
 struct ref_rejection_data {
 	int *retcode;
-	int conflict_msg_shown;
+	bool conflict_msg_shown;
+	bool case_sensitive_msg_shown;
 	const char *remote_name;
 };
 
@@ -1657,11 +1658,25 @@ static void ref_transaction_rejection_handler(const char *refname,
 {
 	struct ref_rejection_data *data = cb_data;
 
-	if (err == REF_TRANSACTION_ERROR_NAME_CONFLICT && !data->conflict_msg_shown) {
+	if (err == REF_TRANSACTION_ERROR_CREATE_EXISTS && ignore_case &&
+	    !data->case_sensitive_msg_shown) {
+		error(_("You're on a case-insensitive filesystem, and the remote you are\n"
+			"trying to fetch from has references that only differ in casing. It\n"
+			"is impossible to store such references with the 'files' backend. You\n"
+			"can either accept this as-is, in which case you won't be able to\n"
+			"store all remote references on disk. Or you can alternatively\n"
+			"migrate your repository to use the 'reftable' backend with the\n"
+			"following command:\n\n    git refs migrate --ref-format=reftable\n\n"
+			"Please keep in mind that not all implementations of Git support this\n"
+			"new format yet. So if you use tools other than Git to access this\n"
+			"repository it may not be an option to migrate to reftables.\n"));
+		data->case_sensitive_msg_shown = true;
+	} else if (err == REF_TRANSACTION_ERROR_NAME_CONFLICT &&
+		   !data->conflict_msg_shown) {
 		error(_("some local refs could not be updated; try running\n"
 			" 'git remote prune %s' to remove any old, conflicting "
 			"branches"), data->remote_name);
-		data->conflict_msg_shown = 1;
+		data->conflict_msg_shown = true;
 	} else {
 		const char *reason = ref_transaction_error_msg(err);
 
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 088b52c740..9f58ea4858 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -776,6 +776,8 @@ 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)
+				ret = REF_TRANSACTION_ERROR_CREATE_EXISTS;
 			goto error_return;
 		}
 	}
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 96648a6e5d..e37a5d83e8 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -2294,6 +2294,30 @@ do
 		)
 	'
 
+	test_expect_success CASE_INSENSITIVE_FS,REFFILES "stdin $type batch-updates existing reference" '
+		git init repo &&
+		test_when_finished "rm -fr repo" &&
+		(
+			cd repo &&
+			test_commit one &&
+			old_head=$(git rev-parse HEAD) &&
+			test_commit two &&
+			head=$(git rev-parse HEAD) &&
+
+			format_command $type "create refs/heads/foo" "$head" >stdin &&
+			format_command $type "create refs/heads/ref" "$old_head" >>stdin &&
+			format_command $type "create refs/heads/Foo" "$old_head" >>stdin &&
+			git update-ref $type --stdin --batch-updates <stdin >stdout &&
+
+			echo $head >expect &&
+			git rev-parse refs/heads/foo >actual &&
+			echo $old_head >expect &&
+			git rev-parse refs/heads/ref >actual &&
+			test_cmp expect actual &&
+			test_grep -q "reference already exists" stdout
+		)
+	'
+
 	test_expect_success "stdin $type batch-updates delete incorrect symbolic ref" '
 		git init repo &&
 		test_when_finished "rm -fr repo" &&
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index ebc696546b..57f60da81b 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -47,7 +47,13 @@ test_expect_success "clone and setup child repos" '
 		git config set branch.main.merge refs/heads/one
 	) &&
 	git clone . bundle &&
-	git clone . seven
+	git clone . seven &&
+	git clone --ref-format=reftable . case_sensitive &&
+	(
+		cd case_sensitive &&
+		git branch branch1 &&
+		git branch bRanch1
+	)
 '
 
 test_expect_success "fetch test" '
@@ -1526,6 +1532,20 @@ test_expect_success SYMLINKS 'clone does not get confused by a D/F conflict' '
 	test_path_is_missing whoops
 '
 
+test_expect_success CASE_INSENSITIVE_FS,REFFILES 'existing references in a case insensitive filesystem' '
+	test_when_finished rm -rf case_insensitive &&
+	(
+		git init --bare case_insensitive &&
+		cd case_insensitive &&
+		git remote add origin -- ../case_sensitive &&
+		test_must_fail git fetch -f origin "refs/heads/*:refs/heads/*" 2>err &&
+		test_grep "You${SQ}re on a case-insensitive filesystem" err &&
+		git rev-parse refs/heads/main >expect &&
+		git rev-parse refs/heads/branch1 >actual &&
+		test_cmp expect actual
+	)
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 

-- 
2.50.1

Re: [PATCH 1/2] refs/files: use correct error type when locking fails

[Chris Torek]

Minor:

On Tue, Sep 2, 2025 at 1:40 AM Karthik Nayak <karthik.188@gmail.com> wrote:
> During the 'prepare' phase of reference transaction in the files
> backend, we create the lock files for references to be created. When
[mass snippage for space]
>
> This is buggy behavior since the user is never intimated about the
> overrides performed and missing references. Nevertheless, the user is

"Intimated" is the wrong word (also this is a rare form, "intimate"
as a verb that is, at least in US and British English). I'd suggest
"informed" here, or some alternate phrasing.

Chris

Re: [PATCH 1/2] refs/files: use correct error type when locking fails

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 697 bytes --]

Chris Torek <chris.torek@gmail.com> writes:

> Minor:
>
> On Tue, Sep 2, 2025 at 1:40 AM Karthik Nayak <karthik.188@gmail.com> wrote:
>> During the 'prepare' phase of reference transaction in the files
>> backend, we create the lock files for references to be created. When
> [mass snippage for space]
>>
>> This is buggy behavior since the user is never intimated about the
>> overrides performed and missing references. Nevertheless, the user is
>
> "Intimated" is the wrong word (also this is a rare form, "intimate"
> as a verb that is, at least in US and British English). I'd suggest
> "informed" here, or some alternate phrasing.
>

Thanks, will swap it out!

> Chris

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 1/2] refs/files: use correct error type when locking fails

[Patrick Steinhardt]

On Tue, Sep 02, 2025 at 10:34:25AM +0200, Karthik Nayak wrote:
> During the 'prepare' phase of reference transaction in the files
> backend, we create the lock files for references to be created. When
> using batched updates on case-insensitive filesystems, the transactions
> would be aborted if there are conflicting names such as:
> 
>   refs/heads/Foo
>   refs/heads/foo
> 
> This affects all commands which were migrated to use batched updates in
> Git 2.51, including 'git-fetch(1)' and 'git-receive-pack(1)'. Before
> that, references updates would be applied serially with one transaction

s/references/reference/

> used per update. When users fetched multiple references on
> case-insensitive systems, subsequent references would simply overwrite
> any earlier references. So when fetching:
> 
>   refs/heads/foo: 5f34ec0bfeac225b1c854340257a65b106f70ea6
>   refs/heads/Foo: ec3053b0977e83d9b67fc32c4527a117953994f3
>   refs/heads/sample: 2eefd1150e06d8fca1ddfa684dec016f36bf4e56
> 
> The user would simply end up with:
> 
>   refs/heads/foo: ec3053b0977e83d9b67fc32c4527a117953994f3
>   refs/heads/sample: 2eefd1150e06d8fca1ddfa684dec016f36bf4e56
> 
> This is buggy behavior since the user is never intimated about the
> overrides performed and missing references. Nevertheless, the user is
> left with a working repository with a subset of the references. Since
> Git 2.51, in such situations fetches would simply fail without applying
> any references. Which is also buggy behavior and worse off since the
> user is left without any references.

Yup, agreed.

> The error is triggered in `lock_raw_ref()` where the files backend
> attempts to create a lock file. When a lock file already exists the
> function returns a 'REF_TRANSACTION_ERROR_GENERIC'. Change this to return
> 'REF_TRANSACTION_ERROR_CREATE_EXISTS' instead to aid the batched update
> mechanism to simply reject such errors.
> 
> This bubbles the error type up to `files_transaction_prepare()` which
> tries to lock each reference update. So if the locking fails, we check
> if the rejection type can be ignored, which is done by calling
> `ref_transaction_maybe_set_rejected()`.
> 
> As the error type is now 'REF_TRANSACTION_ERROR_CREATE_EXISTS', the
> specific reference update would simply be rejected, while other updates
> in the transaction would continue to be applied. This allows partial
> application of references in case-insensitive filesystems when fetching
> colliding references.

Okay. Does that mean that both git-fetch(1) and git-receive-pack(1) are
already told to evict unsuccessful updates? If so, this bit of info
should probably be added to the commit message to say that it was
already the intent, but that it didn't work out because of the
unexpected error type.

> While the earlier implementation allowed the last reference to be
> applied overriding the initial references, this change would allow the
> first reference to be applied while rejecting consequent collisions.
> This should be an OKAY compromise since with the files backend, there is

I don't quite get why we're shouting :) In any case I think the
compromise is acceptable, but we very much should warn the user about
this error. Ideally, we'd even guide them towards the reftable backend.
But let's read on, maybe you already do that.

> no scenario possible where we would retain all colliding references.
> 
> The change only affects batched updates since batched updates will
> reject individual updates with non-generic errors. So specifically this
> would only affect:
> 
>     1. git fetch
>     2. git receive-pack
>     3. git update-ref --batch-updates

Okay, here you mention that we already use batched updates for those
commands. I think it would help the reader if this was explained before
going into the individual error codes.

> Let's also be more pro-active and notify users on case-insensitive
> filesystems about such problems by providing a brief about the issue
> while also recommending using the reftable backend, which doesn't have
> the same issue.

And yup, you already do exactly what I was proposing. Nice!

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 24645c4653..9563abbe12 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1643,7 +1643,8 @@ static int set_head(const struct ref *remote_refs, struct remote *remote)
>  
>  struct ref_rejection_data {
>  	int *retcode;
> -	int conflict_msg_shown;
> +	bool conflict_msg_shown;
> +	bool case_sensitive_msg_shown;
>  	const char *remote_name;
>  };
>  
> @@ -1657,11 +1658,25 @@ static void ref_transaction_rejection_handler(const char *refname,
>  {
>  	struct ref_rejection_data *data = cb_data;
>  
> -	if (err == REF_TRANSACTION_ERROR_NAME_CONFLICT && !data->conflict_msg_shown) {
> +	if (err == REF_TRANSACTION_ERROR_CREATE_EXISTS && ignore_case &&
> +	    !data->case_sensitive_msg_shown) {
> +		error(_("You're on a case-insensitive filesystem, and the remote you are\n"
> +			"trying to fetch from has references that only differ in casing. It\n"
> +			"is impossible to store such references with the 'files' backend. You\n"
> +			"can either accept this as-is, in which case you won't be able to\n"
> +			"store all remote references on disk. Or you can alternatively\n"
> +			"migrate your repository to use the 'reftable' backend with the\n"
> +			"following command:\n\n    git refs migrate --ref-format=reftable\n\n"
> +			"Please keep in mind that not all implementations of Git support this\n"
> +			"new format yet. So if you use tools other than Git to access this\n"
> +			"repository it may not be an option to migrate to reftables.\n"));

This reads familiar :)

> +		data->case_sensitive_msg_shown = true;
> +	} else if (err == REF_TRANSACTION_ERROR_NAME_CONFLICT &&
> +		   !data->conflict_msg_shown) {
>  		error(_("some local refs could not be updated; try running\n"
>  			" 'git remote prune %s' to remove any old, conflicting "
>  			"branches"), data->remote_name);
> -		data->conflict_msg_shown = 1;
> +		data->conflict_msg_shown = true;
>  	} else {
>  		const char *reason = ref_transaction_error_msg(err);
>  
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 088b52c740..9f58ea4858 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -776,6 +776,8 @@ 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)
> +				ret = REF_TRANSACTION_ERROR_CREATE_EXISTS;
>  			goto error_return;
>  		}
>  	}

This here is the actual bug fix that makes us treat the error
gracefully.

> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index 96648a6e5d..e37a5d83e8 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -2294,6 +2294,30 @@ do
>  		)
>  	'
>  
> +	test_expect_success CASE_INSENSITIVE_FS,REFFILES "stdin $type batch-updates existing reference" '
> +		git init repo &&
> +		test_when_finished "rm -fr repo" &&
> +		(
> +			cd repo &&
> +			test_commit one &&
> +			old_head=$(git rev-parse HEAD) &&
> +			test_commit two &&
> +			head=$(git rev-parse HEAD) &&
> +
> +			format_command $type "create refs/heads/foo" "$head" >stdin &&
> +			format_command $type "create refs/heads/ref" "$old_head" >>stdin &&
> +			format_command $type "create refs/heads/Foo" "$old_head" >>stdin &&

These could be written as:

    {
        format_command $type "create refs/heads/foo" "$head" &&
        format_command $type "create refs/heads/ref" "$old_head" &&
        format_command $type "create refs/heads/Foo" "$old_head"
    } >stdin

> +			git update-ref $type --stdin --batch-updates <stdin >stdout &&
> +
> +			echo $head >expect &&
> +			git rev-parse refs/heads/foo >actual &&
> +			echo $old_head >expect &&
> +			git rev-parse refs/heads/ref >actual &&
> +			test_cmp expect actual &&
> +			test_grep -q "reference already exists" stdout
> +		)
> +	'
> +
>  	test_expect_success "stdin $type batch-updates delete incorrect symbolic ref" '
>  		git init repo &&
>  		test_when_finished "rm -fr repo" &&

We could think about making these tests not depend on the REFFILES
prerequisite and then verify that with the reftable backend things work
as expected.

Patrick

Re: [PATCH 1/2] refs/files: use correct error type when locking fails

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 8847 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> On Tue, Sep 02, 2025 at 10:34:25AM +0200, Karthik Nayak wrote:
>> During the 'prepare' phase of reference transaction in the files
>> backend, we create the lock files for references to be created. When
>> using batched updates on case-insensitive filesystems, the transactions
>> would be aborted if there are conflicting names such as:
>>
>>   refs/heads/Foo
>>   refs/heads/foo
>>
>> This affects all commands which were migrated to use batched updates in
>> Git 2.51, including 'git-fetch(1)' and 'git-receive-pack(1)'. Before
>> that, references updates would be applied serially with one transaction
>
> s/references/reference/
>
>> used per update. When users fetched multiple references on
>> case-insensitive systems, subsequent references would simply overwrite
>> any earlier references. So when fetching:
>>
>>   refs/heads/foo: 5f34ec0bfeac225b1c854340257a65b106f70ea6
>>   refs/heads/Foo: ec3053b0977e83d9b67fc32c4527a117953994f3
>>   refs/heads/sample: 2eefd1150e06d8fca1ddfa684dec016f36bf4e56
>>
>> The user would simply end up with:
>>
>>   refs/heads/foo: ec3053b0977e83d9b67fc32c4527a117953994f3
>>   refs/heads/sample: 2eefd1150e06d8fca1ddfa684dec016f36bf4e56
>>
>> This is buggy behavior since the user is never intimated about the
>> overrides performed and missing references. Nevertheless, the user is
>> left with a working repository with a subset of the references. Since
>> Git 2.51, in such situations fetches would simply fail without applying
>> any references. Which is also buggy behavior and worse off since the
>> user is left without any references.
>
> Yup, agreed.
>
>> The error is triggered in `lock_raw_ref()` where the files backend
>> attempts to create a lock file. When a lock file already exists the
>> function returns a 'REF_TRANSACTION_ERROR_GENERIC'. Change this to return
>> 'REF_TRANSACTION_ERROR_CREATE_EXISTS' instead to aid the batched update
>> mechanism to simply reject such errors.
>>
>> This bubbles the error type up to `files_transaction_prepare()` which
>> tries to lock each reference update. So if the locking fails, we check
>> if the rejection type can be ignored, which is done by calling
>> `ref_transaction_maybe_set_rejected()`.
>>
>> As the error type is now 'REF_TRANSACTION_ERROR_CREATE_EXISTS', the
>> specific reference update would simply be rejected, while other updates
>> in the transaction would continue to be applied. This allows partial
>> application of references in case-insensitive filesystems when fetching
>> colliding references.
>
> Okay. Does that mean that both git-fetch(1) and git-receive-pack(1) are
> already told to evict unsuccessful updates? If so, this bit of info
> should probably be added to the commit message to say that it was
> already the intent, but that it didn't work out because of the
> unexpected error type.
>
>> While the earlier implementation allowed the last reference to be
>> applied overriding the initial references, this change would allow the
>> first reference to be applied while rejecting consequent collisions.
>> This should be an OKAY compromise since with the files backend, there is
>
> I don't quite get why we're shouting :) In any case I think the
> compromise is acceptable, but we very much should warn the user about
> this error. Ideally, we'd even guide them towards the reftable backend.
> But let's read on, maybe you already do that.
>

Let me remove that shout :D

>> no scenario possible where we would retain all colliding references.
>>
>> The change only affects batched updates since batched updates will
>> reject individual updates with non-generic errors. So specifically this
>> would only affect:
>>
>>     1. git fetch
>>     2. git receive-pack
>>     3. git update-ref --batch-updates
>
> Okay, here you mention that we already use batched updates for those
> commands. I think it would help the reader if this was explained before
> going into the individual error codes.
>

Yeah it would read better earlier on, let me move it around.

>> Let's also be more pro-active and notify users on case-insensitive
>> filesystems about such problems by providing a brief about the issue
>> while also recommending using the reftable backend, which doesn't have
>> the same issue.
>
> And yup, you already do exactly what I was proposing. Nice!
>

It was copied from your suggestion!

>> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> index 24645c4653..9563abbe12 100644
>> --- a/builtin/fetch.c
>> +++ b/builtin/fetch.c
>> @@ -1643,7 +1643,8 @@ static int set_head(const struct ref *remote_refs, struct remote *remote)
>>
>>  struct ref_rejection_data {
>>  	int *retcode;
>> -	int conflict_msg_shown;
>> +	bool conflict_msg_shown;
>> +	bool case_sensitive_msg_shown;
>>  	const char *remote_name;
>>  };
>>
>> @@ -1657,11 +1658,25 @@ static void ref_transaction_rejection_handler(const char *refname,
>>  {
>>  	struct ref_rejection_data *data = cb_data;
>>
>> -	if (err == REF_TRANSACTION_ERROR_NAME_CONFLICT && !data->conflict_msg_shown) {
>> +	if (err == REF_TRANSACTION_ERROR_CREATE_EXISTS && ignore_case &&
>> +	    !data->case_sensitive_msg_shown) {
>> +		error(_("You're on a case-insensitive filesystem, and the remote you are\n"
>> +			"trying to fetch from has references that only differ in casing. It\n"
>> +			"is impossible to store such references with the 'files' backend. You\n"
>> +			"can either accept this as-is, in which case you won't be able to\n"
>> +			"store all remote references on disk. Or you can alternatively\n"
>> +			"migrate your repository to use the 'reftable' backend with the\n"
>> +			"following command:\n\n    git refs migrate --ref-format=reftable\n\n"
>> +			"Please keep in mind that not all implementations of Git support this\n"
>> +			"new format yet. So if you use tools other than Git to access this\n"
>> +			"repository it may not be an option to migrate to reftables.\n"));
>
> This reads familiar :)
>

Which I failed to attribute to you, sorry for missing that, will add in
a 'Helped-by'.

>> +		data->case_sensitive_msg_shown = true;
>> +	} else if (err == REF_TRANSACTION_ERROR_NAME_CONFLICT &&
>> +		   !data->conflict_msg_shown) {
>>  		error(_("some local refs could not be updated; try running\n"
>>  			" 'git remote prune %s' to remove any old, conflicting "
>>  			"branches"), data->remote_name);
>> -		data->conflict_msg_shown = 1;
>> +		data->conflict_msg_shown = true;
>>  	} else {
>>  		const char *reason = ref_transaction_error_msg(err);
>>
>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index 088b52c740..9f58ea4858 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -776,6 +776,8 @@ 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)
>> +				ret = REF_TRANSACTION_ERROR_CREATE_EXISTS;
>>  			goto error_return;
>>  		}
>>  	}
>
> This here is the actual bug fix that makes us treat the error
> gracefully.
>
>> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
>> index 96648a6e5d..e37a5d83e8 100755
>> --- a/t/t1400-update-ref.sh
>> +++ b/t/t1400-update-ref.sh
>> @@ -2294,6 +2294,30 @@ do
>>  		)
>>  	'
>>
>> +	test_expect_success CASE_INSENSITIVE_FS,REFFILES "stdin $type batch-updates existing reference" '
>> +		git init repo &&
>> +		test_when_finished "rm -fr repo" &&
>> +		(
>> +			cd repo &&
>> +			test_commit one &&
>> +			old_head=$(git rev-parse HEAD) &&
>> +			test_commit two &&
>> +			head=$(git rev-parse HEAD) &&
>> +
>> +			format_command $type "create refs/heads/foo" "$head" >stdin &&
>> +			format_command $type "create refs/heads/ref" "$old_head" >>stdin &&
>> +			format_command $type "create refs/heads/Foo" "$old_head" >>stdin &&
>
> These could be written as:
>
>     {
>         format_command $type "create refs/heads/foo" "$head" &&
>         format_command $type "create refs/heads/ref" "$old_head" &&
>         format_command $type "create refs/heads/Foo" "$old_head"
>     } >stdin
>

That reads much better, thanks.

>> +			git update-ref $type --stdin --batch-updates <stdin >stdout &&
>> +
>> +			echo $head >expect &&
>> +			git rev-parse refs/heads/foo >actual &&
>> +			echo $old_head >expect &&
>> +			git rev-parse refs/heads/ref >actual &&
>> +			test_cmp expect actual &&
>> +			test_grep -q "reference already exists" stdout
>> +		)
>> +	'
>> +
>>  	test_expect_success "stdin $type batch-updates delete incorrect symbolic ref" '
>>  		git init repo &&
>>  		test_when_finished "rm -fr repo" &&
>
> We could think about making these tests not depend on the REFFILES
> prerequisite and then verify that with the reftable backend things work
> as expected.
>
> Patrick

That would be a good test to add, will add it in the next version.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 1/2] refs/files: use correct error type when locking fails

[Patrick Steinhardt]

On Wed, Sep 03, 2025 at 03:38:04AM -0700, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > On Tue, Sep 02, 2025 at 10:34:25AM +0200, Karthik Nayak wrote:
> >> diff --git a/builtin/fetch.c b/builtin/fetch.c
> >> index 24645c4653..9563abbe12 100644
> >> --- a/builtin/fetch.c
> >> +++ b/builtin/fetch.c
> >> @@ -1657,11 +1658,25 @@ static void ref_transaction_rejection_handler(const char *refname,
> >>  {
> >>  	struct ref_rejection_data *data = cb_data;
> >>
> >> -	if (err == REF_TRANSACTION_ERROR_NAME_CONFLICT && !data->conflict_msg_shown) {
> >> +	if (err == REF_TRANSACTION_ERROR_CREATE_EXISTS && ignore_case &&
> >> +	    !data->case_sensitive_msg_shown) {
> >> +		error(_("You're on a case-insensitive filesystem, and the remote you are\n"
> >> +			"trying to fetch from has references that only differ in casing. It\n"
> >> +			"is impossible to store such references with the 'files' backend. You\n"
> >> +			"can either accept this as-is, in which case you won't be able to\n"
> >> +			"store all remote references on disk. Or you can alternatively\n"
> >> +			"migrate your repository to use the 'reftable' backend with the\n"
> >> +			"following command:\n\n    git refs migrate --ref-format=reftable\n\n"
> >> +			"Please keep in mind that not all implementations of Git support this\n"
> >> +			"new format yet. So if you use tools other than Git to access this\n"
> >> +			"repository it may not be an option to migrate to reftables.\n"));
> >
> > This reads familiar :)
> >
> 
> Which I failed to attribute to you, sorry for missing that, will add in
> a 'Helped-by'.

No worries, I didn't mind it at all.

Patrick

Re: [PATCH 1/2] refs/files: use correct error type when locking fails

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> During the 'prepare' phase of reference transaction in the files
> backend, we create the lock files for references to be created. When
> using batched updates on case-insensitive filesystems, the transactions
> would be aborted if there are conflicting names such as:
>
>   refs/heads/Foo
>   refs/heads/foo
>
> This affects all commands which were migrated to use batched updates in
> Git 2.51, including 'git-fetch(1)' and 'git-receive-pack(1)'. Before
> that, references updates would be applied serially with one transaction
> used per update. When users fetched multiple references on
> case-insensitive systems, subsequent references would simply overwrite
> any earlier references. So when fetching:
>
>   refs/heads/foo: 5f34ec0bfeac225b1c854340257a65b106f70ea6
>   refs/heads/Foo: ec3053b0977e83d9b67fc32c4527a117953994f3
>   refs/heads/sample: 2eefd1150e06d8fca1ddfa684dec016f36bf4e56
>
> The user would simply end up with:
>
>   refs/heads/foo: ec3053b0977e83d9b67fc32c4527a117953994f3
>   refs/heads/sample: 2eefd1150e06d8fca1ddfa684dec016f36bf4e56
>
> This is buggy behavior since the user is never intimated about the

"intimated" -> "informed" or simply "told".

> overrides performed and missing references. Nevertheless, the user is
> left with a working repository with a subset of the references. Since
> Git 2.51, in such situations fetches would simply fail without applying

"applying" -> "updating".

> any references. Which is also buggy behavior and worse off since the
> user is left without any references.

Very true.

> The error is triggered in `lock_raw_ref()` where the files backend
> attempts to create a lock file. When a lock file already exists the
> function returns a 'REF_TRANSACTION_ERROR_GENERIC'. Change this to return
> 'REF_TRANSACTION_ERROR_CREATE_EXISTS' instead to aid the batched update
> mechanism to simply reject such errors.

In the above description, both "batched" and "transaction" are used
but they mean different things and their difference is critical to
this description, right?  IIUC, the mechanism for "batched updates"
is based on the transaction mechanism where all-or-none is the norm,
and when in batched mode, that all-or-none-ness that makes it a
transaction is deliberately broken and lets certain types of errors
cause operations on refs individually rejected.

After "The error is triggerred...a REF_TRANSACTION_ERROR_GENERIC"
but before "Change this", you would want to say what the code does
(i.e. "When this happens, the entire batched updates, not individual
operation, is aborted as if it were in a transaction") to highlight
why you would want to "Change this", wouldn't you?

> While the earlier implementation allowed the last reference to be
> applied overriding the initial references, this change would allow the
> first reference to be applied while rejecting consequent collisions.
> This should be an OKAY compromise since with the files backend, there is
> no scenario possible where we would retain all colliding references.

OK.  How do we know that a existing lockfile on a case insensitive
filesystem can only be due to somebody tried to lock a ref that is
only different in case, and not a leftover lockfile or lockfile held
by some competing process?  Don't we _know_ all the refs that are
involved in _our_ batched update when we find that we failed to lock
one particular ref?  We can inspect other refs we have locked so far
(assuming that the transaction mechanism knows what refs it is
updating) and see if one of them is truly conflicting only in case,
and if the code did so, I am happy if the code ignored that lock
failure (and the ref update).  But I feel a bit uneasy to see that
any "ah there already is _somebody_ holding a lock on this ref"
without checking that it is _we_ that took the lock for another ref
whose path is only different in case and ignoring the failure.

> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 088b52c740..9f58ea4858 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -776,6 +776,8 @@ 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)
> +				ret = REF_TRANSACTION_ERROR_CREATE_EXISTS;
>  			goto error_return;

I guess the place to check would be here in EEXIST case.  Since it
is an error codepath, we can afford to be more careful (probably
with an out-of-line logic implemented in a helper function call made
from here).

Thanks.

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

[Karthik Nayak]

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.

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.

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 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
 

-- 
2.50.1

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

[Patrick Steinhardt]

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.

atrick

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

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 4750 bytes --]

Patrick Steinhardt <ps@pks.im> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

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

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> 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...

> 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

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> 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..

Here is a quick-and-dirty monkey-see-monkey-do patch on top of your
series, and with it jobs on osx boxes seem to fail.

https://github.com/git/git/actions/runs/17477662700/job/49641377250#step:4:1912

 t/t5510-fetch.sh | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 84dc68e5f3..76af769f5c 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -59,6 +59,12 @@ test_expect_success "clone and setup child repos" '
 		cd case_sensitive_fd &&
 		git branch foo/bar &&
 		git branch Foo
+	) &&
+	git clone --ref-format=reftable . case_sensitive_df &&
+	(
+		cd case_sensitive_df &&
+		git branch Foo/bar &&
+		git branch foo
 	)
 '
 
@@ -1566,6 +1572,20 @@ test_expect_success CASE_INSENSITIVE_FS,REFFILES 'F/D conflict on case insensiti
 	)
 '
 
+test_expect_success CASE_INSENSITIVE_FS,REFFILES 'D/F 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_df &&
+		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
 
-- 
2.51.0-314-g9a15cfd6dc

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

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 2237 bytes --]

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> 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.
>

I totally agree, it shouldn't know/care about specific reference backends.

>> 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".
>

Totally fair. Fixed.

>> ... 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..
>

Thanks for pointing it out and also providing a test. That indeed is a
problem, but that is an issue caught much earlier, creating 'foo', when
'Foo/bar' exists, fails at the locking step, since the locking step also
checks for D/F conflicts. I've added another commit around this, fixing
the check that you mentioned.

>> Reported-by: Junio C Hamano <gitster@pobox.com>
>
> Hmph, I do not recall reporting anything, but perhaps it was a long
> time ago...
>

[snip]

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

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

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 455 bytes --]

Karthik Nayak <karthik.188@gmail.com> writes:

[snip]

>>> Reported-by: Junio C Hamano <gitster@pobox.com>
>>
>> Hmph, I do not recall reporting anything, but perhaps it was a long
>> time ago...
>>
>

Reported-by might not be the right field here, You mentioned about the
D/F conflict in an earlier mail [1]. I would've missed it otherwise. So
I wanted to attribute you for that :)

[1]: https://lore.kernel.org/all/xmqq8qjbxn8n.fsf@gitster.g/

> [snip]

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

[PATCH v2 0/4] refs/files: fix issues with git-fetch on case-insensitive FS

[Karthik Nayak]

Hello!

With Git 2.51 we moved 'git-fetch(1)' and 'git-receive-pack(1)' to use
batched updates while doing reference updates. This provided a nice perf
boost since both commands will now use a single transaction for
reference updates. This removes the overhead of using individual
transaction per reference update and also avoids unnecessary
auto-compaction between reference updates in the reftable backend.

However, in the files-backend it does introduce a few bugs around
conflicts. The reported bug was around case-insensitive filesystems [1],
but we also fix some adjacent issues:

1. When fetching references such as:

   - refs/heads/foo
   - refs/heads/Foo

Earlier we would simply overwrite the first reference with the second
and continue. Since Git 2.51 we simply abort stating a conflict.

This is resolved in the first commit by explicitly categorizing the
error as non-GENERIC. This allows batched updates to reject the
particular update, while updating the rest.

2. When fetching references and a lock for a particular reference
already exits. We treat this is a GENERIC error, which fails the entire
update. By categorizing this error as non-GENERIC, we can reject this
specific update and update the other references.

3. When fetching references such as with F/D conflict:

  - refs/heads/foo
  - refs/heads/Foo/bar

Earlier we would apply the first, while the second would fail due to
conflict. Since Git 2.51, the lock files for both would be created, but
the 'commit' phase would abruptly end leaving the lock files.

The second commit fixes this by ensuring that on case-insensitive
filesystems we lowercase the refnames for availability check to ensure
F/D are caught and reported to the user.

4. When fetching references with D/F conflict:

  - refs/heads/Foo/bar
  - refs/heads/foo

The creation of the second reference's lock in `lock_raw_ref()` catches
the D/F conflict, but we mark this as a GENERIC error. By categorizing
this as non-GENERIC, we can allow the updates to continue while
rejecting this specific error.

- Karthik

[1]: https://lore.kernel.org/all/YQXPR01MB3046197EF39296549EE6DD669A33A@YQXPR01MB3046.CANPRD01.PROD.OUTLOOK.COM/

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
Changes in v2:
- This version fixes two more issues:
  - Fetching while locks already exist in the repository
  - D/F conflicts while fetching
- Add a specific error to the first case, so we can nicely show a
  relevant error. Also check explicitly that the issue is due to
  case-insensitive filesystems.
- Cleanup the commit messages.
- Use `string_list_append_nodup()` with `strbuf_detach`, reducing the
  number of allocations.

---
 builtin/fetch.c       | 21 ++++++++++--
 refs.c                | 10 +++++-
 refs.h                |  2 ++
 refs/files-backend.c  | 51 ++++++++++++++++++++++++-----
 t/t1400-update-ref.sh | 53 +++++++++++++++++++++++++++++++
 t/t5510-fetch.sh      | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 6 files changed, 212 insertions(+), 13 deletions(-)

Karthik Nayak (4):
      refs/files: catch conflicts on case-insensitive file-systems
      refs/files: use correct error type when lock exists
      refs/files: handle F/D conflicts in case-insensitive FS
      refs/files: handle D/F conflicts during locking

Range-diff versus v1:

1:  fe6e2c12e7 ! 1:  bab864e28a refs/files: use correct error type when locking fails
    @@ Metadata
     Author: Karthik Nayak <karthik.188@gmail.com>
     
      ## Commit message ##
    -    refs/files: use correct error type when locking fails
    +    refs/files: catch conflicts on case-insensitive file-systems
     
         During the 'prepare' phase of reference transaction in the files
         backend, we create the lock files for references to be created. When
    -    using batched updates on case-insensitive filesystems, the transactions
    -    would be aborted if there are conflicting names such as:
    +    using batched updates on case-insensitive filesystems, the entire
    +    batched updates would be aborted if there are conflicting names such as:
     
           refs/heads/Foo
           refs/heads/foo
     
         This affects all commands which were migrated to use batched updates in
         Git 2.51, including 'git-fetch(1)' and 'git-receive-pack(1)'. Before
    -    that, references updates would be applied serially with one transaction
    +    that, reference updates would be applied serially with one transaction
         used per update. When users fetched multiple references on
         case-insensitive systems, subsequent references would simply overwrite
         any earlier references. So when fetching:
    @@ Commit message
           refs/heads/foo: ec3053b0977e83d9b67fc32c4527a117953994f3
           refs/heads/sample: 2eefd1150e06d8fca1ddfa684dec016f36bf4e56
     
    -    This is buggy behavior since the user is never intimated about the
    +    This is buggy behavior since the user is never informed about the
         overrides performed and missing references. Nevertheless, the user is
         left with a working repository with a subset of the references. Since
    -    Git 2.51, in such situations fetches would simply fail without applying
    +    Git 2.51, in such situations fetches would simply fail without updating
         any references. Which is also buggy behavior and worse off since the
         user is left without any references.
     
         The error is triggered in `lock_raw_ref()` where the files backend
         attempts to create a lock file. When a lock file already exists the
    -    function returns a 'REF_TRANSACTION_ERROR_GENERIC'. Change this to return
    -    'REF_TRANSACTION_ERROR_CREATE_EXISTS' instead to aid the batched update
    -    mechanism to simply reject such errors.
    +    function returns a 'REF_TRANSACTION_ERROR_GENERIC'. When this happens,
    +    the entire batched updates, not individual operation, is aborted as if
    +    it were in a transaction.
    +
    +    Change this to return 'REF_TRANSACTION_ERROR_CASE_CONFLICT' instead to
    +    aid the batched update mechanism to simply reject such errors. The
    +    change only affects batched updates since batched updates will reject
    +    individual updates with non-generic errors. So specifically this would
    +    only affect:
    +
    +        1. git fetch
    +        2. git receive-pack
    +        3. git update-ref --batch-updates
     
         This bubbles the error type up to `files_transaction_prepare()` which
         tries to lock each reference update. So if the locking fails, we check
         if the rejection type can be ignored, which is done by calling
         `ref_transaction_maybe_set_rejected()`.
     
    -    As the error type is now 'REF_TRANSACTION_ERROR_CREATE_EXISTS', the
    -    specific reference update would simply be rejected, while other updates
    -    in the transaction would continue to be applied. This allows partial
    -    application of references in case-insensitive filesystems when fetching
    -    colliding references.
    +    As the error type is now 'REF_TRANSACTION_ERROR_CASE_CONFLICT',
    +    the specific reference update would simply be rejected, while other
    +    updates in the transaction would continue to be applied. This allows
    +    partial application of references in case-insensitive filesystems when
    +    fetching colliding references.
     
         While the earlier implementation allowed the last reference to be
         applied overriding the initial references, this change would allow the
         first reference to be applied while rejecting consequent collisions.
    -    This should be an OKAY compromise since with the files backend, there is
    +    This should be an okay compromise since with the files backend, there is
         no scenario possible where we would retain all colliding references.
     
    -    The change only affects batched updates since batched updates will
    -    reject individual updates with non-generic errors. So specifically this
    -    would only affect:
    -
    -        1. git fetch
    -        2. git receive-pack
    -        3. git update-ref --batch-updates
    -
         Let's also be more pro-active and notify users on case-insensitive
         filesystems about such problems by providing a brief about the issue
         while also recommending using the reftable backend, which doesn't have
         the same issue.
     
         Reported-by: Joe Drew <joe.drew@indexexchange.com>
    +    Helped-by: Patrick Steinhardt <ps@pks.im>
         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
     
      ## builtin/fetch.c ##
    @@ builtin/fetch.c: static void ref_transaction_rejection_handler(const char *refna
      	struct ref_rejection_data *data = cb_data;
      
     -	if (err == REF_TRANSACTION_ERROR_NAME_CONFLICT && !data->conflict_msg_shown) {
    -+	if (err == REF_TRANSACTION_ERROR_CREATE_EXISTS && ignore_case &&
    ++	if (err == REF_TRANSACTION_ERROR_CASE_CONFLICT && ignore_case &&
     +	    !data->case_sensitive_msg_shown) {
     +		error(_("You're on a case-insensitive filesystem, and the remote you are\n"
     +			"trying to fetch from has references that only differ in casing. It\n"
    @@ builtin/fetch.c: static void ref_transaction_rejection_handler(const char *refna
      		const char *reason = ref_transaction_error_msg(err);
      
     
    + ## refs.c ##
    +@@ refs.c: const char *ref_transaction_error_msg(enum ref_transaction_error err)
    + 		return "invalid new value provided";
    + 	case REF_TRANSACTION_ERROR_EXPECTED_SYMREF:
    + 		return "expected symref but found regular ref";
    ++	case REF_TRANSACTION_ERROR_CASE_CONFLICT:
    ++		return "reference conflict due to case-insensitive filesystem";
    + 	default:
    + 		return "unknown failure";
    + 	}
    +
    + ## refs.h ##
    +@@ refs.h: 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,
    + };
    + 
    + /*
    +
      ## refs/files-backend.c ##
    +@@ refs/files-backend.c: static void unlock_ref(struct ref_lock *lock)
    + 	}
    + }
    + 
    ++static bool duplicate_reference_case_cmp(struct ref_transaction *transaction,
    ++					 struct ref_update *update)
    ++{
    ++	for (size_t i = 0; i < transaction->nr; i++) {
    ++		if (transaction->updates[i] == update)
    ++			break;
    ++
    ++		if (!strcasecmp(transaction->updates[i]->refname, update->refname))
    ++			return true;
    ++	}
    ++	return false;
    ++}
    ++
    + /*
    +  * Lock refname, without following symrefs, and set *lock_p to point
    +  * at a newly-allocated lock object. Fill in lock->old_oid, referent,
    +@@ refs/files-backend.c: static void unlock_ref(struct ref_lock *lock)
    +  * - Generate informative error messages in the case of failure
    +  */
    + static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs,
    +-					       struct ref_update *update,
    ++					       struct ref_transaction *transaction,
    + 					       size_t update_idx,
    + 					       int mustexist,
    + 					       struct string_list *refnames_to_check,
    +-					       const struct string_list *extras,
    + 					       struct ref_lock **lock_p,
    + 					       struct strbuf *referent,
    + 					       struct strbuf *err)
    + {
    + 	enum ref_transaction_error ret = REF_TRANSACTION_ERROR_GENERIC;
    ++	struct ref_update *update = transaction->updates[update_idx];
    ++	const struct string_list *extras = &transaction->refnames;
    + 	const char *refname = update->refname;
    + 	unsigned int *type = &update->type;
    + 	struct ref_lock *lock;
     @@ refs/files-backend.c: 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)
    -+				ret = REF_TRANSACTION_ERROR_CREATE_EXISTS;
    ++			if (myerr == EEXIST && ignore_case &&
    ++			    duplicate_reference_case_cmp(transaction, update))
    ++				ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
      			goto error_return;
      		}
      	}
    +@@ refs/files-backend.c: static enum ref_transaction_error lock_ref_for_update(struct files_ref_store *re
    + 	if (lock) {
    + 		lock->count++;
    + 	} else {
    +-		ret = lock_raw_ref(refs, update, update_idx, mustexist,
    +-				   refnames_to_check, &transaction->refnames,
    +-				   &lock, &referent, err);
    ++		ret = lock_raw_ref(refs, transaction, update_idx, mustexist,
    ++				   refnames_to_check, &lock, &referent, err);
    + 		if (ret) {
    + 			char *reason;
    + 
     
      ## t/t1400-update-ref.sh ##
     @@ t/t1400-update-ref.sh: do
    @@ t/t1400-update-ref.sh: do
     +			test_commit two &&
     +			head=$(git rev-parse HEAD) &&
     +
    -+			format_command $type "create refs/heads/foo" "$head" >stdin &&
    -+			format_command $type "create refs/heads/ref" "$old_head" >>stdin &&
    -+			format_command $type "create refs/heads/Foo" "$old_head" >>stdin &&
    ++			{
    ++				format_command $type "create refs/heads/foo" "$head" &&
    ++				format_command $type "create refs/heads/ref" "$old_head" &&
    ++				format_command $type "create refs/heads/Foo" "$old_head"
    ++			} >stdin &&
    ++			git update-ref $type --stdin --batch-updates <stdin >stdout &&
    ++
    ++			echo $head >expect &&
    ++			git rev-parse refs/heads/foo >actual &&
    ++			echo $old_head >expect &&
    ++			git rev-parse refs/heads/ref >actual &&
    ++			test_cmp expect actual &&
    ++			test_grep -q "reference conflict due to case-insensitive filesystem" stdout
    ++		)
    ++	'
    ++
    ++	test_expect_success CASE_INSENSITIVE_FS "stdin $type batch-updates existing reference" '
    ++		git init --ref-format=reftable repo &&
    ++		test_when_finished "rm -fr repo" &&
    ++		(
    ++			cd repo &&
    ++			test_commit one &&
    ++			old_head=$(git rev-parse HEAD) &&
    ++			test_commit two &&
    ++			head=$(git rev-parse HEAD) &&
    ++
    ++			{
    ++				format_command $type "create refs/heads/foo" "$head" &&
    ++				format_command $type "create refs/heads/ref" "$old_head" &&
    ++				format_command $type "create refs/heads/Foo" "$old_head"
    ++			} >stdin &&
     +			git update-ref $type --stdin --batch-updates <stdin >stdout &&
     +
     +			echo $head >expect &&
    @@ t/t1400-update-ref.sh: do
     +			echo $old_head >expect &&
     +			git rev-parse refs/heads/ref >actual &&
     +			test_cmp expect actual &&
    -+			test_grep -q "reference already exists" stdout
    ++			git rev-parse refs/heads/Foo >actual &&
    ++			test_cmp expect actual
     +		)
     +	'
     +
-:  ---------- > 2:  b0ecf6f10d refs/files: use correct error type when lock exists
2:  30a2629ebc ! 3:  1842ddee90 refs/files: handle F/D conflicts in case-insensitive FS
    @@ Metadata
      ## Commit message ##
         refs/files: handle F/D conflicts in case-insensitive FS
     
    -    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:
    +    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'
    @@ Commit message
         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.
    +    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
    +    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.
    @@ refs/files-backend.c: static enum ref_transaction_error lock_raw_ref(struct file
     +			strbuf_addstr(&lower, refname);
     +			strbuf_tolower(&lower);
     +
    -+			item = string_list_append(refnames_to_check, lower.buf);
    -+			strbuf_release(&lower);
    ++			item = string_list_append_nodup(refnames_to_check,
    ++							strbuf_detach(&lower, NULL));
     +		} else {
     +			item = string_list_append(refnames_to_check, refname);
     +		}
    @@ t/t5510-fetch.sh: test_expect_success "clone and setup child repos" '
      	)
      '
      
    -@@ t/t5510-fetch.sh: test_expect_success CASE_INSENSITIVE_FS,REFFILES 'existing references in a case
    +@@ t/t5510-fetch.sh: test_expect_success REFFILES 'existing reference lock in repo' '
      	)
      '
      
-:  ---------- > 4:  81759b0e51 refs/files: handle D/F conflicts during locking


base-commit: c44beea485f0f2feaf460e2ac87fdd5608d63cf0
change-id: 20250822-587-git-fetch-1-fails-fetches-on-case-insensitive-repositories-0a187c7ac41e

Thanks
- Karthik

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

[Karthik Nayak]

During the 'prepare' phase of reference transaction in the files
backend, we create the lock files for references to be created. When
using batched updates on case-insensitive filesystems, the entire
batched updates would be aborted if there are conflicting names such as:

  refs/heads/Foo
  refs/heads/foo

This affects all commands which were migrated to use batched updates in
Git 2.51, including 'git-fetch(1)' and 'git-receive-pack(1)'. Before
that, reference updates would be applied serially with one transaction
used per update. When users fetched multiple references on
case-insensitive systems, subsequent references would simply overwrite
any earlier references. So when fetching:

  refs/heads/foo: 5f34ec0bfeac225b1c854340257a65b106f70ea6
  refs/heads/Foo: ec3053b0977e83d9b67fc32c4527a117953994f3
  refs/heads/sample: 2eefd1150e06d8fca1ddfa684dec016f36bf4e56

The user would simply end up with:

  refs/heads/foo: ec3053b0977e83d9b67fc32c4527a117953994f3
  refs/heads/sample: 2eefd1150e06d8fca1ddfa684dec016f36bf4e56

This is buggy behavior since the user is never informed about the
overrides performed and missing references. Nevertheless, the user is
left with a working repository with a subset of the references. Since
Git 2.51, in such situations fetches would simply fail without updating
any references. Which is also buggy behavior and worse off since the
user is left without any references.

The error is triggered in `lock_raw_ref()` where the files backend
attempts to create a lock file. When a lock file already exists the
function returns a 'REF_TRANSACTION_ERROR_GENERIC'. When this happens,
the entire batched updates, not individual operation, is aborted as if
it were in a transaction.

Change this to return 'REF_TRANSACTION_ERROR_CASE_CONFLICT' instead to
aid the batched update mechanism to simply reject such errors. The
change only affects batched updates since batched updates will reject
individual updates with non-generic errors. So specifically this would
only affect:

    1. git fetch
    2. git receive-pack
    3. git update-ref --batch-updates

This bubbles the error type up to `files_transaction_prepare()` which
tries to lock each reference update. So if the locking fails, we check
if the rejection type can be ignored, which is done by calling
`ref_transaction_maybe_set_rejected()`.

As the error type is now 'REF_TRANSACTION_ERROR_CASE_CONFLICT',
the specific reference update would simply be rejected, while other
updates in the transaction would continue to be applied. This allows
partial application of references in case-insensitive filesystems when
fetching colliding references.

While the earlier implementation allowed the last reference to be
applied overriding the initial references, this change would allow the
first reference to be applied while rejecting consequent collisions.
This should be an okay compromise since with the files backend, there is
no scenario possible where we would retain all colliding references.

Let's also be more pro-active and notify users on case-insensitive
filesystems about such problems by providing a brief about the issue
while also recommending using the reftable backend, which doesn't have
the same issue.

Reported-by: Joe Drew <joe.drew@indexexchange.com>
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/fetch.c       | 21 +++++++++++++++++---
 refs.c                |  2 ++
 refs.h                |  2 ++
 refs/files-backend.c  | 26 ++++++++++++++++++++-----
 t/t1400-update-ref.sh | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
 t/t5510-fetch.sh      | 22 ++++++++++++++++++++-
 6 files changed, 117 insertions(+), 9 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 24645c4653..c7ff3480fb 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1643,7 +1643,8 @@ static int set_head(const struct ref *remote_refs, struct remote *remote)
 
 struct ref_rejection_data {
 	int *retcode;
-	int conflict_msg_shown;
+	bool conflict_msg_shown;
+	bool case_sensitive_msg_shown;
 	const char *remote_name;
 };
 
@@ -1657,11 +1658,25 @@ static void ref_transaction_rejection_handler(const char *refname,
 {
 	struct ref_rejection_data *data = cb_data;
 
-	if (err == REF_TRANSACTION_ERROR_NAME_CONFLICT && !data->conflict_msg_shown) {
+	if (err == REF_TRANSACTION_ERROR_CASE_CONFLICT && ignore_case &&
+	    !data->case_sensitive_msg_shown) {
+		error(_("You're on a case-insensitive filesystem, and the remote you are\n"
+			"trying to fetch from has references that only differ in casing. It\n"
+			"is impossible to store such references with the 'files' backend. You\n"
+			"can either accept this as-is, in which case you won't be able to\n"
+			"store all remote references on disk. Or you can alternatively\n"
+			"migrate your repository to use the 'reftable' backend with the\n"
+			"following command:\n\n    git refs migrate --ref-format=reftable\n\n"
+			"Please keep in mind that not all implementations of Git support this\n"
+			"new format yet. So if you use tools other than Git to access this\n"
+			"repository it may not be an option to migrate to reftables.\n"));
+		data->case_sensitive_msg_shown = true;
+	} else if (err == REF_TRANSACTION_ERROR_NAME_CONFLICT &&
+		   !data->conflict_msg_shown) {
 		error(_("some local refs could not be updated; try running\n"
 			" 'git remote prune %s' to remove any old, conflicting "
 			"branches"), data->remote_name);
-		data->conflict_msg_shown = 1;
+		data->conflict_msg_shown = true;
 	} else {
 		const char *reason = ref_transaction_error_msg(err);
 
diff --git a/refs.c b/refs.c
index bfdbe718b7..4c1c339ed9 100644
--- a/refs.c
+++ b/refs.c
@@ -3321,6 +3321,8 @@ const char *ref_transaction_error_msg(enum ref_transaction_error err)
 		return "invalid new value provided";
 	case REF_TRANSACTION_ERROR_EXPECTED_SYMREF:
 		return "expected symref but found regular ref";
+	case REF_TRANSACTION_ERROR_CASE_CONFLICT:
+		return "reference conflict due to case-insensitive filesystem";
 	default:
 		return "unknown failure";
 	}
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,
 };
 
 /*
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)
+{
+	for (size_t i = 0; i < transaction->nr; i++) {
+		if (transaction->updates[i] == update)
+			break;
+
+		if (!strcasecmp(transaction->updates[i]->refname, update->refname))
+			return true;
+	}
+	return false;
+}
+
 /*
  * Lock refname, without following symrefs, and set *lock_p to point
  * at a newly-allocated lock object. Fill in lock->old_oid, referent,
@@ -677,16 +690,17 @@ static void unlock_ref(struct ref_lock *lock)
  * - Generate informative error messages in the case of failure
  */
 static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs,
-					       struct ref_update *update,
+					       struct ref_transaction *transaction,
 					       size_t update_idx,
 					       int mustexist,
 					       struct string_list *refnames_to_check,
-					       const struct string_list *extras,
 					       struct ref_lock **lock_p,
 					       struct strbuf *referent,
 					       struct strbuf *err)
 {
 	enum ref_transaction_error ret = REF_TRANSACTION_ERROR_GENERIC;
+	struct ref_update *update = transaction->updates[update_idx];
+	const struct string_list *extras = &transaction->refnames;
 	const char *refname = update->refname;
 	unsigned int *type = &update->type;
 	struct ref_lock *lock;
@@ -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;
 		}
 	}
@@ -2583,9 +2600,8 @@ static enum ref_transaction_error lock_ref_for_update(struct files_ref_store *re
 	if (lock) {
 		lock->count++;
 	} else {
-		ret = lock_raw_ref(refs, update, update_idx, mustexist,
-				   refnames_to_check, &transaction->refnames,
-				   &lock, &referent, err);
+		ret = lock_raw_ref(refs, transaction, update_idx, mustexist,
+				   refnames_to_check, &lock, &referent, err);
 		if (ret) {
 			char *reason;
 
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 96648a6e5d..08d5df2af7 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -2294,6 +2294,59 @@ do
 		)
 	'
 
+	test_expect_success CASE_INSENSITIVE_FS,REFFILES "stdin $type batch-updates existing reference" '
+		git init repo &&
+		test_when_finished "rm -fr repo" &&
+		(
+			cd repo &&
+			test_commit one &&
+			old_head=$(git rev-parse HEAD) &&
+			test_commit two &&
+			head=$(git rev-parse HEAD) &&
+
+			{
+				format_command $type "create refs/heads/foo" "$head" &&
+				format_command $type "create refs/heads/ref" "$old_head" &&
+				format_command $type "create refs/heads/Foo" "$old_head"
+			} >stdin &&
+			git update-ref $type --stdin --batch-updates <stdin >stdout &&
+
+			echo $head >expect &&
+			git rev-parse refs/heads/foo >actual &&
+			echo $old_head >expect &&
+			git rev-parse refs/heads/ref >actual &&
+			test_cmp expect actual &&
+			test_grep -q "reference conflict due to case-insensitive filesystem" stdout
+		)
+	'
+
+	test_expect_success CASE_INSENSITIVE_FS "stdin $type batch-updates existing reference" '
+		git init --ref-format=reftable repo &&
+		test_when_finished "rm -fr repo" &&
+		(
+			cd repo &&
+			test_commit one &&
+			old_head=$(git rev-parse HEAD) &&
+			test_commit two &&
+			head=$(git rev-parse HEAD) &&
+
+			{
+				format_command $type "create refs/heads/foo" "$head" &&
+				format_command $type "create refs/heads/ref" "$old_head" &&
+				format_command $type "create refs/heads/Foo" "$old_head"
+			} >stdin &&
+			git update-ref $type --stdin --batch-updates <stdin >stdout &&
+
+			echo $head >expect &&
+			git rev-parse refs/heads/foo >actual &&
+			echo $old_head >expect &&
+			git rev-parse refs/heads/ref >actual &&
+			test_cmp expect actual &&
+			git rev-parse refs/heads/Foo >actual &&
+			test_cmp expect actual
+		)
+	'
+
 	test_expect_success "stdin $type batch-updates delete incorrect symbolic ref" '
 		git init repo &&
 		test_when_finished "rm -fr repo" &&
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index ebc696546b..57f60da81b 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -47,7 +47,13 @@ test_expect_success "clone and setup child repos" '
 		git config set branch.main.merge refs/heads/one
 	) &&
 	git clone . bundle &&
-	git clone . seven
+	git clone . seven &&
+	git clone --ref-format=reftable . case_sensitive &&
+	(
+		cd case_sensitive &&
+		git branch branch1 &&
+		git branch bRanch1
+	)
 '
 
 test_expect_success "fetch test" '
@@ -1526,6 +1532,20 @@ test_expect_success SYMLINKS 'clone does not get confused by a D/F conflict' '
 	test_path_is_missing whoops
 '
 
+test_expect_success CASE_INSENSITIVE_FS,REFFILES 'existing references in a case insensitive filesystem' '
+	test_when_finished rm -rf case_insensitive &&
+	(
+		git init --bare case_insensitive &&
+		cd case_insensitive &&
+		git remote add origin -- ../case_sensitive &&
+		test_must_fail git fetch -f origin "refs/heads/*:refs/heads/*" 2>err &&
+		test_grep "You${SQ}re on a case-insensitive filesystem" err &&
+		git rev-parse refs/heads/main >expect &&
+		git rev-parse refs/heads/branch1 >actual &&
+		test_cmp expect actual
+	)
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 

-- 
2.50.1

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

[Patrick Steinhardt]

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.

> 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()`?

> +{
> +	for (size_t i = 0; i < transaction->nr; i++) {
> +		if (transaction->updates[i] == update)
> +			break;

Why do we break here? Shouldn't we continue?

> @@ -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?

Patrick

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

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 2534 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> 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.

>> 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.

>> +{
>> +	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.

>> @@ -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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

[PATCH v2 2/4] refs/files: use correct error type when lock exists

[Karthik Nayak]

When fetching references into a repository, if a lock for a particular
reference exists, then `lock_raw_ref()` throws the generic error
'REF_TRANSACTION_ERROR_GENERIC'. This causes the entire set of batched
updates to fail.

Instead, return a 'REF_TRANSACTION_ERROR_CREATE_EXISTS' error. This
allows batched updates to reject the individual update which conflicts
with the existing file, while updating the rest of the references.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs/files-backend.c | 10 +++++++---
 t/t5510-fetch.sh     | 26 ++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 58005d2732..2730713d23 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -790,9 +790,13 @@ 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;
+			if (myerr == EEXIST) {
+				if (ignore_case && duplicate_reference_case_cmp(transaction, update))
+					ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
+				else
+					ret = REF_TRANSACTION_ERROR_CREATE_EXISTS;
+			}
+
 			goto error_return;
 		}
 	}
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 57f60da81b..6f8db0ace4 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -1546,6 +1546,32 @@ test_expect_success CASE_INSENSITIVE_FS,REFFILES 'existing references in a case
 	)
 '
 
+test_expect_success REFFILES 'existing reference lock in repo' '
+	test_when_finished rm -rf base repo &&
+	(
+		git init --ref-format=reftable base &&
+		cd base &&
+		echo >file update &&
+		git add . &&
+		git commit -m "updated" &&
+		git branch -M main &&
+
+		git update-ref refs/heads/foo @ &&
+		git update-ref refs/heads/branch @ &&
+		cd .. &&
+
+		git init --ref-format=files --bare repo &&
+		cd repo &&
+		git remote add origin ../base &&
+		touch refs/heads/foo.lock &&
+		test_must_fail git fetch -f origin "refs/heads/*:refs/heads/*" 2>err &&
+		test_grep "error: fetching ref refs/heads/foo failed: reference already exists" err &&
+		git rev-parse refs/heads/main >expect &&
+		git rev-parse refs/heads/branch >actual &&
+		test_cmp expect actual
+	)
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 

-- 
2.50.1

Re: [PATCH v2 2/4] refs/files: use correct error type when lock exists

[Patrick Steinhardt]

On Mon, Sep 08, 2025 at 02:37:36PM +0200, Karthik Nayak wrote:
> When fetching references into a repository, if a lock for a particular
> reference exists, then `lock_raw_ref()` throws the generic error
> 'REF_TRANSACTION_ERROR_GENERIC'. This causes the entire set of batched
> updates to fail.

This isn't quite accurate anymore as we may also raise
`REF_TRANSACTION_ERROR_CASE_CONFLICT` now.

> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 58005d2732..2730713d23 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -790,9 +790,13 @@ 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;
> +			if (myerr == EEXIST) {
> +				if (ignore_case && duplicate_reference_case_cmp(transaction, update))
> +					ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
> +				else
> +					ret = REF_TRANSACTION_ERROR_CREATE_EXISTS;
> +			}
> +
>  			goto error_return;
>  		}
>  	}

Hm. So if I understand correctly, we now return CREATE_EXISTS in case we
have a conflict with a preexisting case-conflicting reference, but we
return CASE_CONFLICT in case we have a conflict with an update in the
same transaction?

It feels awkward, but I guess that's the best thing we can do. We
happily overwrite case-conflicting preexisting refs, so we wouldn't even
see EEXIST in that case. So the only case where we still see that error
is on a D/F conflict, and in that case it makes sense to return
CREATE_EXISTS.

I feel like this should be added as a comment though, as it gives some
important non-obvious context.

Patrick

Re: [PATCH v2 2/4] refs/files: use correct error type when lock exists

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 2162 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> On Mon, Sep 08, 2025 at 02:37:36PM +0200, Karthik Nayak wrote:
>> When fetching references into a repository, if a lock for a particular
>> reference exists, then `lock_raw_ref()` throws the generic error
>> 'REF_TRANSACTION_ERROR_GENERIC'. This causes the entire set of batched
>> updates to fail.
>
> This isn't quite accurate anymore as we may also raise
> `REF_TRANSACTION_ERROR_CASE_CONFLICT` now.
>

Good catch, will fixup.

>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index 58005d2732..2730713d23 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -790,9 +790,13 @@ 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;
>> +			if (myerr == EEXIST) {
>> +				if (ignore_case && duplicate_reference_case_cmp(transaction, update))
>> +					ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
>> +				else
>> +					ret = REF_TRANSACTION_ERROR_CREATE_EXISTS;
>> +			}
>> +
>>  			goto error_return;
>>  		}
>>  	}
>
> Hm. So if I understand correctly, we now return CREATE_EXISTS in case we
> have a conflict with a preexisting case-conflicting reference, but we
> return CASE_CONFLICT in case we have a conflict with an update in the
> same transaction?
>

It's not a reference, rather the lock that causes the conflict. So a
preexisting case-conflicting reference lock would raise the GENERIC
error. Which would fail all updates.

> It feels awkward, but I guess that's the best thing we can do. We
> happily overwrite case-conflicting preexisting refs, so we wouldn't even
> see EEXIST in that case. So the only case where we still see that error
> is on a D/F conflict, and in that case it makes sense to return
> CREATE_EXISTS.
>
> I feel like this should be added as a comment though, as it gives some
> important non-obvious context.
>

Yeah, it would be beneficial, will add some comments here.

> Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

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

[Karthik Nayak]

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
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.

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 2730713d23..85f2e14e93 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -888,8 +888,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));
+		} else {
+			item = string_list_append(refnames_to_check, refname);
+		}
+
 		item->util = xmalloc(sizeof(update_idx));
 		memcpy(item->util, &update_idx, sizeof(update_idx));
 	}
@@ -2814,7 +2829,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 6f8db0ace4..08dbea6503 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
 	)
 '
 
@@ -1572,6 +1578,20 @@ test_expect_success REFFILES 'existing reference lock in repo' '
 	)
 '
 
+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
 

-- 
2.50.1

[PATCH v2 4/4] refs/files: handle D/F conflicts during locking

[Karthik Nayak]

The previous commit, added the necessary validation and checks for F/D
conflicts in the files backend when working on case insensitive systems.

There is still a possibility for D/F conflicts. This is a different from
the F/D since for F/D conflicts, there would not be a conflict during
the lock creation phase:

    refs/heads/foo.lock
    refs/heads/foo/bar.lock

However there would be a conflict when the locks are committed, since we
cannot have 'refs/heads/foo/bar' and 'refs/heads/foo'. These kinds of
conflicts are checked and resolved in
`refs_verify_refnames_available()`, so the previous commit ensured that
for case-insensitive filesystems, we would lowercase the inputs to that
function.

For D/F conflicts, there is a conflict during the lock creation phase
itself:

    refs/heads/foo/bar.lock
    refs/heads/foo.lock

As in `lock_raw_ref()` after creating the lock, we also check for D/F
conflicts. To fix this, simply categorize the error as a name conflict.
Also remove this reference from the list of valid refnames for
availability checks.

By categorizing the error and removing it from the list of valid
references, batched updates now knows to reject such reference updates
and apply the other reference updates.

Fix a small typo in `ref_transaction_maybe_set_rejected()` while here.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs.c               |  8 +++++++-
 refs/files-backend.c |  2 +-
 t/t5510-fetch.sh     | 20 ++++++++++++++++++++
 3 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index 4c1c339ed9..ec4f0e9502 100644
--- a/refs.c
+++ b/refs.c
@@ -1223,7 +1223,7 @@ int ref_transaction_maybe_set_rejected(struct ref_transaction *transaction,
 		return 0;
 
 	if (!transaction->rejections)
-		BUG("transaction not inititalized with failure support");
+		BUG("transaction not initialized with failure support");
 
 	/*
 	 * Don't accept generic errors, since these errors are not user
@@ -1232,6 +1232,12 @@ int ref_transaction_maybe_set_rejected(struct ref_transaction *transaction,
 	if (err == REF_TRANSACTION_ERROR_GENERIC)
 		return 0;
 
+	/*
+	 * Remove this refname from the list of refnames used for validation
+	 */
+	string_list_remove(&transaction->refnames,
+			   transaction->updates[update_idx]->refname, 0);
+
 	transaction->updates[update_idx]->rejection_err = err;
 	ALLOC_GROW(transaction->rejections->update_indices,
 		   transaction->rejections->nr + 1,
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 85f2e14e93..ceeec272ff 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -852,6 +852,7 @@ static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs,
 				goto error_return;
 			} else if (remove_dir_recursively(&ref_file,
 							  REMOVE_DIR_EMPTY_ONLY)) {
+				ret = REF_TRANSACTION_ERROR_NAME_CONFLICT;
 				if (refs_verify_refname_available(
 						    &refs->base, refname,
 						    extras, NULL, 0, err)) {
@@ -859,7 +860,6 @@ static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs,
 					 * The error message set by
 					 * verify_refname_available() is OK.
 					 */
-					ret = REF_TRANSACTION_ERROR_NAME_CONFLICT;
 					goto error_return;
 				} else {
 					/*
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 08dbea6503..ffaa956cde 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -59,6 +59,12 @@ test_expect_success "clone and setup child repos" '
 		cd case_sensitive_fd &&
 		git branch foo/bar &&
 		git branch Foo
+	) &&
+	git clone --ref-format=reftable . case_sensitive_df &&
+	(
+		cd case_sensitive_df &&
+		git branch Foo/bar &&
+		git branch foo
 	)
 '
 
@@ -1592,6 +1598,20 @@ test_expect_success CASE_INSENSITIVE_FS,REFFILES 'F/D conflict on case insensiti
 	)
 '
 
+test_expect_success CASE_INSENSITIVE_FS,REFFILES 'D/F 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_df &&
+		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
 

-- 
2.50.1

Re: [PATCH v2 4/4] refs/files: handle D/F conflicts during locking

[Patrick Steinhardt]

On Mon, Sep 08, 2025 at 02:37:38PM +0200, Karthik Nayak wrote:
> The previous commit, added the necessary validation and checks for F/D

s/commit,/commit/

> diff --git a/refs.c b/refs.c
> index 4c1c339ed9..ec4f0e9502 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1232,6 +1232,12 @@ int ref_transaction_maybe_set_rejected(struct ref_transaction *transaction,
>  	if (err == REF_TRANSACTION_ERROR_GENERIC)
>  		return 0;
>  
> +	/*
> +	 * Remove this refname from the list of refnames used for validation
> +	 */

Nit: it's obvious that we remove the refname from that list, so the
comment is not helping much. It's much more important to explain _why_
we do that though to give readers the necessary context.

> +	string_list_remove(&transaction->refnames,
> +			   transaction->updates[update_idx]->refname, 0);
> +
>  	transaction->updates[update_idx]->rejection_err = err;
>  	ALLOC_GROW(transaction->rejections->update_indices,
>  		   transaction->rejections->nr + 1,
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 85f2e14e93..ceeec272ff 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -852,6 +852,7 @@ static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs,
>  				goto error_return;
>  			} else if (remove_dir_recursively(&ref_file,
>  							  REMOVE_DIR_EMPTY_ONLY)) {
> +				ret = REF_TRANSACTION_ERROR_NAME_CONFLICT;
>  				if (refs_verify_refname_available(
>  						    &refs->base, refname,
>  						    extras, NULL, 0, err)) {
> @@ -859,7 +860,6 @@ static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs,
>  					 * The error message set by
>  					 * verify_refname_available() is OK.
>  					 */
> -					ret = REF_TRANSACTION_ERROR_NAME_CONFLICT;
>  					goto error_return;
>  				} else {
>  					/*

Hm, interesting. Previously we'd have returned a generic error in the
`else` branch, which reads like this:

	} else {
		/*
		 * We can't delete the directory,
		 * but we also don't know of any
		 * references that it should
		 * contain.
		 */
		strbuf_addf(err, "there is a non-empty directory '%s' "
			    "blocking reference '%s'",
			    ref_file.buf, refname);
		goto error_return;
	}

So that directory contains something, even though we've previously
verified that it shouldn't, if I understand correctly. The test case you
add does seem to indicate that there are valid cases though where this
can happen on a case insensitive filesystem?

If so, the comment definitely needs to be updated to explain this
additional error case.

One more question is whether it's the correct thing to unconditionally
return REF_TRANSACTION_ERROR_NAME_CONFLICT in that case now. Could it be
that the directory exists but contains some garbage?

Patrick

Re: [PATCH v2 4/4] refs/files: handle D/F conflicts during locking

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 3554 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> On Mon, Sep 08, 2025 at 02:37:38PM +0200, Karthik Nayak wrote:
>> The previous commit, added the necessary validation and checks for F/D
>
> s/commit,/commit/
>

Thanks, will change.

>> diff --git a/refs.c b/refs.c
>> index 4c1c339ed9..ec4f0e9502 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -1232,6 +1232,12 @@ int ref_transaction_maybe_set_rejected(struct ref_transaction *transaction,
>>  	if (err == REF_TRANSACTION_ERROR_GENERIC)
>>  		return 0;
>>
>> +	/*
>> +	 * Remove this refname from the list of refnames used for validation
>> +	 */
>
> Nit: it's obvious that we remove the refname from that list, so the
> comment is not helping much. It's much more important to explain _why_
> we do that though to give readers the necessary context.
>

Indeed, I'll add this instead

  Rejected refnames shouldn't be considered in the availability checks,
  so remove them from the list.

>> +	string_list_remove(&transaction->refnames,
>> +			   transaction->updates[update_idx]->refname, 0);
>> +
>>  	transaction->updates[update_idx]->rejection_err = err;
>>  	ALLOC_GROW(transaction->rejections->update_indices,
>>  		   transaction->rejections->nr + 1,
>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index 85f2e14e93..ceeec272ff 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -852,6 +852,7 @@ static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs,
>>  				goto error_return;
>>  			} else if (remove_dir_recursively(&ref_file,
>>  							  REMOVE_DIR_EMPTY_ONLY)) {
>> +				ret = REF_TRANSACTION_ERROR_NAME_CONFLICT;
>>  				if (refs_verify_refname_available(
>>  						    &refs->base, refname,
>>  						    extras, NULL, 0, err)) {
>> @@ -859,7 +860,6 @@ static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs,
>>  					 * The error message set by
>>  					 * verify_refname_available() is OK.
>>  					 */
>> -					ret = REF_TRANSACTION_ERROR_NAME_CONFLICT;
>>  					goto error_return;
>>  				} else {
>>  					/*
>
> Hm, interesting. Previously we'd have returned a generic error in the
> `else` branch, which reads like this:
>
> 	} else {
> 		/*
> 		 * We can't delete the directory,
> 		 * but we also don't know of any
> 		 * references that it should
> 		 * contain.
> 		 */
> 		strbuf_addf(err, "there is a non-empty directory '%s' "
> 			    "blocking reference '%s'",
> 			    ref_file.buf, refname);
> 		goto error_return;
> 	}
>
> So that directory contains something, even though we've previously
> verified that it shouldn't, if I understand correctly. The test case you
> add does seem to indicate that there are valid cases though where this
> can happen on a case insensitive filesystem?
>
> If so, the comment definitely needs to be updated to explain this
> additional error case.
>

Yeah I think that comment needs to be rewritten.

> One more question is whether it's the correct thing to unconditionally
> return REF_TRANSACTION_ERROR_NAME_CONFLICT in that case now. Could it be
> that the directory exists but contains some garbage?
>
> Patrick

This does affect case-sensitive systems too. I should've added a test
there as well to showcase this.

Basically if there is a lock file in a directory
'refs/heads/foo/bar.lock' while fetching 'refs/heads/foo', this would
reach this section of the code. Pre batched-updates, we would skip this
ref and update other refs. So it makes sense to retain this behavior.
I'll add in a test and amend the commit message to reflect this
behavior.

- Karthik

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

[PATCH v3 0/4] refs/files: fix issues with git-fetch on case-insensitive FS

[Karthik Nayak]

Hello!

With Git 2.51 we moved 'git-fetch(1)' and 'git-receive-pack(1)' to use
batched updates while doing reference updates. This provided a nice perf
boost since both commands will now use a single transaction for
reference updates. This removes the overhead of using individual
transaction per reference update and also avoids unnecessary
auto-compaction between reference updates in the reftable backend.

However, in the files-backend it does introduce a few bugs around
conflicts. The reported bug was around case-insensitive filesystems [1],
but we also fix some adjacent issues:

1. When fetching references such as:

   - refs/heads/foo
   - refs/heads/Foo

Earlier we would simply overwrite the first reference with the second
and continue. Since Git 2.51 we simply abort stating a conflict.

This is resolved in the first commit by explicitly categorizing the
error as non-GENERIC. This allows batched updates to reject the
particular update, while updating the rest.

2. When fetching references and a lock for a particular reference
already exits. We treat this is a GENERIC error, which fails the entire
update. By categorizing this error as non-GENERIC, we can reject this
specific update and update the other references.

3. When fetching references such as with F/D conflict:

  - refs/heads/foo
  - refs/heads/Foo/bar

Earlier we would apply the first, while the second would fail due to
conflict. Since Git 2.51, the lock files for both would be created, but
the 'commit' phase would abruptly end leaving the lock files.

The second commit fixes this by ensuring that on case-insensitive
filesystems we lowercase the refnames for availability check to ensure
F/D are caught and reported to the user.

4. When fetching references with D/F conflict:

  - refs/heads/Foo/bar
  - refs/heads/foo

The creation of the second reference's lock in `lock_raw_ref()` catches
the D/F conflict, but we mark this as a GENERIC error. By categorizing
this as non-GENERIC, we can allow the updates to continue while
rejecting this specific error.

This also applies to D/F conflicts in case-sensitive systems where there
exists a lock in the repo 'refs/heads/foo/bar.lock' causing a conflict
while fetching 'refs/heads/foo'.

- Karthik

[1]: https://lore.kernel.org/all/YQXPR01MB3046197EF39296549EE6DD669A33A@YQXPR01MB3046.CANPRD01.PROD.OUTLOOK.COM/

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
Changes in v3:
- Rename duplicate_reference_case_cmp() to
  transaction_has_case_conflicting_update() and add comments.
- Improve commit messages.
- Add an additional test in the 4th commit to showcase D/F conflicts in
  case-sensistive file systems.
- Link to v2: https://lore.kernel.org/r/20250908-587-git-fetch-1-fails-fetches-on-case-insensitive-repositories-v2-0-b2eb2459befb@gmail.com

Changes in v2:
- This version fixes two more issues:
  - Fetching while locks already exist in the repository
  - D/F conflicts while fetching
- Add a specific error to the first case, so we can nicely show a
  relevant error. Also check explicitly that the issue is due to
  case-insensitive filesystems.
- Cleanup the commit messages.
- Use `string_list_append_nodup()` with `strbuf_detach`, reducing the
  number of allocations.

---
 builtin/fetch.c       |  21 ++++++++--
 refs.c                |  11 ++++-
 refs.h                |   2 +
 refs/files-backend.c  |  77 ++++++++++++++++++++++++++++------
 t/t1400-update-ref.sh |  53 +++++++++++++++++++++++
 t/t5510-fetch.sh      | 114 +++++++++++++++++++++++++++++++++++++++++++++++++-
 6 files changed, 261 insertions(+), 17 deletions(-)

Karthik Nayak (4):
      refs/files: catch conflicts on case-insensitive file-systems
      refs/files: use correct error type when lock exists
      refs/files: handle F/D conflicts in case-insensitive FS
      refs/files: handle D/F conflicts during locking

Range-diff versus v2:

1:  570b24fc09 ! 1:  1d4ff64d90 refs/files: catch conflicts on case-insensitive file-systems
    @@ refs/files-backend.c: static void unlock_ref(struct ref_lock *lock)
      	}
      }
      
    -+static bool duplicate_reference_case_cmp(struct ref_transaction *transaction,
    -+					 struct ref_update *update)
    ++/*
    ++ * Check if the transaction has another update with a case-insensitive refname
    ++ * match.
    ++ *
    ++ * If the update is part of the transaction, we only check up to that index.
    ++ * Further updates are expected to call this function to match previous indices.
    ++ */
    ++static bool transaction_has_case_conflicting_update(struct ref_transaction *transaction,
    ++						    struct ref_update *update)
     +{
     +	for (size_t i = 0; i < transaction->nr; i++) {
     +		if (transaction->updates[i] == update)
    @@ refs/files-backend.c: static enum ref_transaction_error lock_raw_ref(struct file
      		} else {
      			unable_to_lock_message(ref_file.buf, myerr, err);
     +			if (myerr == EEXIST && ignore_case &&
    -+			    duplicate_reference_case_cmp(transaction, update))
    ++			    transaction_has_case_conflicting_update(transaction, update))
     +				ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
      			goto error_return;
      		}
2:  10bfb8c1f2 ! 2:  fdc69b233e refs/files: use correct error type when lock exists
    @@ Commit message
         refs/files: use correct error type when lock exists
     
         When fetching references into a repository, if a lock for a particular
    -    reference exists, then `lock_raw_ref()` throws the generic error
    -    'REF_TRANSACTION_ERROR_GENERIC'. This causes the entire set of batched
    -    updates to fail.
    +    reference exists, then `lock_raw_ref()` throws:
    +
    +        - REF_TRANSACTION_ERROR_CASE_CONFLICT: when there is a conflict
    +        because transaction contains conflicting references while being on a
    +        case-insensitive filesystem.
    +
    +        - REF_TRANSACTION_ERROR_GENERIC: for all other errors.
    +
    +    The latter causes the entire set of batched updates to fail, even in
    +    case sensitive filessystems.
     
         Instead, return a 'REF_TRANSACTION_ERROR_CREATE_EXISTS' error. This
         allows batched updates to reject the individual update which conflicts
    @@ refs/files-backend.c: static enum ref_transaction_error lock_raw_ref(struct file
      		} else {
      			unable_to_lock_message(ref_file.buf, myerr, err);
     -			if (myerr == EEXIST && ignore_case &&
    --			    duplicate_reference_case_cmp(transaction, update))
    +-			    transaction_has_case_conflicting_update(transaction, update))
     -				ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
     +			if (myerr == EEXIST) {
    -+				if (ignore_case && duplicate_reference_case_cmp(transaction, update))
    ++				if (ignore_case &&
    ++				    transaction_has_case_conflicting_update(transaction, update))
    ++					/*
    ++					 * In case-insensitive filesystems, ensure that conflicts within a
    ++					 * given transaction are handled. Pre-existing refs on a
    ++					 * case-insensitive system will be overridden without any issue.
    ++					 */
     +					ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
     +				else
    ++					/*
    ++					 * Pre-existing case-conflicting reference locks should also be
    ++					 * specially categorized to avoid failing all batched updates.
    ++					 */
     +					ret = REF_TRANSACTION_ERROR_CREATE_EXISTS;
     +			}
     +
3:  23868f2218 = 3:  ff7fb95b66 refs/files: handle F/D conflicts in case-insensitive FS
4:  5af3dc2a27 ! 4:  a610f9478b refs/files: handle D/F conflicts during locking
    @@ Metadata
      ## Commit message ##
         refs/files: handle D/F conflicts during locking
     
    -    The previous commit, added the necessary validation and checks for F/D
    +    The previous commit added the necessary validation and checks for F/D
         conflicts in the files backend when working on case insensitive systems.
     
         There is still a possibility for D/F conflicts. This is a different from
    @@ Commit message
             refs/heads/foo.lock
     
         As in `lock_raw_ref()` after creating the lock, we also check for D/F
    -    conflicts. To fix this, simply categorize the error as a name conflict.
    -    Also remove this reference from the list of valid refnames for
    -    availability checks.
    +    conflicts. This can occur in case-insensitive filesystems when trying to
    +    fetch case-conflicted references like:
     
    +        refs/heads/Foo/new
    +        refs/heads/foo
    +
    +    D/F conflicts can also occur in case-sensitive filesystems, when the
    +    repository already contains a directory with a lock file
    +    'refs/heads/foo/bar.lock' and trying to fetch 'refs/heads/foo'. This
    +    doesn't concern directories containing garbage files as those are
    +    handled on a higher level.
    +
    +    To fix this, simply categorize the error as a name conflict. Also remove
    +    this reference from the list of valid refnames for availability checks.
         By categorizing the error and removing it from the list of valid
         references, batched updates now knows to reject such reference updates
         and apply the other reference updates.
    @@ refs.c: int ref_transaction_maybe_set_rejected(struct ref_transaction *transacti
      		return 0;
      
     +	/*
    -+	 * Remove this refname from the list of refnames used for validation
    ++	 * Rejected refnames shouldn't be considered in the availability
    ++	 * checks, so remove them from the list.
     +	 */
     +	string_list_remove(&transaction->refnames,
     +			   transaction->updates[update_idx]->refname, 0);
    @@ refs/files-backend.c: static enum ref_transaction_error lock_raw_ref(struct file
      					goto error_return;
      				} else {
      					/*
    +-					 * We can't delete the directory,
    +-					 * but we also don't know of any
    +-					 * references that it should
    +-					 * contain.
    ++					 * Directory conflicts can occur if there
    ++					 * is an existing lock file in the directory
    ++					 * or if the filesystem is case-insensitive
    ++					 * and the directory contains a valid reference
    ++					 * but conflicts with the update.
    + 					 */
    + 					strbuf_addf(err, "there is a non-empty directory '%s' "
    + 						    "blocking reference '%s'",
     
      ## t/t5510-fetch.sh ##
     @@ t/t5510-fetch.sh: test_expect_success "clone and setup child repos" '
    @@ t/t5510-fetch.sh: test_expect_success CASE_INSENSITIVE_FS,REFFILES 'F/D conflict
     +		test_cmp expect actual
     +	)
     +'
    ++
    ++test_expect_success REFFILES 'D/F conflict on case sensitive filesystem with lock' '
    ++	(
    ++		git init --ref-format=reftable base &&
    ++		cd base &&
    ++		echo >file update &&
    ++		git add . &&
    ++		git commit -m "updated" &&
    ++		git branch -M main &&
    ++
    ++		git update-ref refs/heads/foo @ &&
    ++		git update-ref refs/heads/branch @ &&
    ++		cd .. &&
    ++
    ++		git init --ref-format=files --bare repo &&
    ++		cd repo &&
    ++		git remote add origin ../base &&
    ++		mkdir refs/heads/foo &&
    ++		touch refs/heads/foo/random.lock &&
    ++		test_must_fail git fetch origin "refs/heads/*:refs/heads/*" 2>err &&
    ++		test_grep "some local refs could not be updated; try running" err &&
    ++		git rev-parse refs/heads/main >expect &&
    ++		git rev-parse refs/heads/branch >actual &&
    ++		test_cmp expect actual
    ++	)
    ++'
     +
      . "$TEST_DIRECTORY"/lib-httpd.sh
      start_httpd


base-commit: c44beea485f0f2feaf460e2ac87fdd5608d63cf0
change-id: 20250822-587-git-fetch-1-fails-fetches-on-case-insensitive-repositories-0a187c7ac41e

Thanks
- Karthik

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

[Karthik Nayak]

During the 'prepare' phase of reference transaction in the files
backend, we create the lock files for references to be created. When
using batched updates on case-insensitive filesystems, the entire
batched updates would be aborted if there are conflicting names such as:

  refs/heads/Foo
  refs/heads/foo

This affects all commands which were migrated to use batched updates in
Git 2.51, including 'git-fetch(1)' and 'git-receive-pack(1)'. Before
that, reference updates would be applied serially with one transaction
used per update. When users fetched multiple references on
case-insensitive systems, subsequent references would simply overwrite
any earlier references. So when fetching:

  refs/heads/foo: 5f34ec0bfeac225b1c854340257a65b106f70ea6
  refs/heads/Foo: ec3053b0977e83d9b67fc32c4527a117953994f3
  refs/heads/sample: 2eefd1150e06d8fca1ddfa684dec016f36bf4e56

The user would simply end up with:

  refs/heads/foo: ec3053b0977e83d9b67fc32c4527a117953994f3
  refs/heads/sample: 2eefd1150e06d8fca1ddfa684dec016f36bf4e56

This is buggy behavior since the user is never informed about the
overrides performed and missing references. Nevertheless, the user is
left with a working repository with a subset of the references. Since
Git 2.51, in such situations fetches would simply fail without updating
any references. Which is also buggy behavior and worse off since the
user is left without any references.

The error is triggered in `lock_raw_ref()` where the files backend
attempts to create a lock file. When a lock file already exists the
function returns a 'REF_TRANSACTION_ERROR_GENERIC'. When this happens,
the entire batched updates, not individual operation, is aborted as if
it were in a transaction.

Change this to return 'REF_TRANSACTION_ERROR_CASE_CONFLICT' instead to
aid the batched update mechanism to simply reject such errors. The
change only affects batched updates since batched updates will reject
individual updates with non-generic errors. So specifically this would
only affect:

    1. git fetch
    2. git receive-pack
    3. git update-ref --batch-updates

This bubbles the error type up to `files_transaction_prepare()` which
tries to lock each reference update. So if the locking fails, we check
if the rejection type can be ignored, which is done by calling
`ref_transaction_maybe_set_rejected()`.

As the error type is now 'REF_TRANSACTION_ERROR_CASE_CONFLICT',
the specific reference update would simply be rejected, while other
updates in the transaction would continue to be applied. This allows
partial application of references in case-insensitive filesystems when
fetching colliding references.

While the earlier implementation allowed the last reference to be
applied overriding the initial references, this change would allow the
first reference to be applied while rejecting consequent collisions.
This should be an okay compromise since with the files backend, there is
no scenario possible where we would retain all colliding references.

Let's also be more pro-active and notify users on case-insensitive
filesystems about such problems by providing a brief about the issue
while also recommending using the reftable backend, which doesn't have
the same issue.

Reported-by: Joe Drew <joe.drew@indexexchange.com>
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/fetch.c       | 21 +++++++++++++++++---
 refs.c                |  2 ++
 refs.h                |  2 ++
 refs/files-backend.c  | 33 +++++++++++++++++++++++++++-----
 t/t1400-update-ref.sh | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
 t/t5510-fetch.sh      | 22 ++++++++++++++++++++-
 6 files changed, 124 insertions(+), 9 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 24645c4653..c7ff3480fb 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1643,7 +1643,8 @@ static int set_head(const struct ref *remote_refs, struct remote *remote)
 
 struct ref_rejection_data {
 	int *retcode;
-	int conflict_msg_shown;
+	bool conflict_msg_shown;
+	bool case_sensitive_msg_shown;
 	const char *remote_name;
 };
 
@@ -1657,11 +1658,25 @@ static void ref_transaction_rejection_handler(const char *refname,
 {
 	struct ref_rejection_data *data = cb_data;
 
-	if (err == REF_TRANSACTION_ERROR_NAME_CONFLICT && !data->conflict_msg_shown) {
+	if (err == REF_TRANSACTION_ERROR_CASE_CONFLICT && ignore_case &&
+	    !data->case_sensitive_msg_shown) {
+		error(_("You're on a case-insensitive filesystem, and the remote you are\n"
+			"trying to fetch from has references that only differ in casing. It\n"
+			"is impossible to store such references with the 'files' backend. You\n"
+			"can either accept this as-is, in which case you won't be able to\n"
+			"store all remote references on disk. Or you can alternatively\n"
+			"migrate your repository to use the 'reftable' backend with the\n"
+			"following command:\n\n    git refs migrate --ref-format=reftable\n\n"
+			"Please keep in mind that not all implementations of Git support this\n"
+			"new format yet. So if you use tools other than Git to access this\n"
+			"repository it may not be an option to migrate to reftables.\n"));
+		data->case_sensitive_msg_shown = true;
+	} else if (err == REF_TRANSACTION_ERROR_NAME_CONFLICT &&
+		   !data->conflict_msg_shown) {
 		error(_("some local refs could not be updated; try running\n"
 			" 'git remote prune %s' to remove any old, conflicting "
 			"branches"), data->remote_name);
-		data->conflict_msg_shown = 1;
+		data->conflict_msg_shown = true;
 	} else {
 		const char *reason = ref_transaction_error_msg(err);
 
diff --git a/refs.c b/refs.c
index bfdbe718b7..4c1c339ed9 100644
--- a/refs.c
+++ b/refs.c
@@ -3321,6 +3321,8 @@ const char *ref_transaction_error_msg(enum ref_transaction_error err)
 		return "invalid new value provided";
 	case REF_TRANSACTION_ERROR_EXPECTED_SYMREF:
 		return "expected symref but found regular ref";
+	case REF_TRANSACTION_ERROR_CASE_CONFLICT:
+		return "reference conflict due to case-insensitive filesystem";
 	default:
 		return "unknown failure";
 	}
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,
 };
 
 /*
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 088b52c740..01df32904b 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -647,6 +647,26 @@ static void unlock_ref(struct ref_lock *lock)
 	}
 }
 
+/*
+ * Check if the transaction has another update with a case-insensitive refname
+ * match.
+ *
+ * If the update is part of the transaction, we only check up to that index.
+ * Further updates are expected to call this function to match previous indices.
+ */
+static bool transaction_has_case_conflicting_update(struct ref_transaction *transaction,
+						    struct ref_update *update)
+{
+	for (size_t i = 0; i < transaction->nr; i++) {
+		if (transaction->updates[i] == update)
+			break;
+
+		if (!strcasecmp(transaction->updates[i]->refname, update->refname))
+			return true;
+	}
+	return false;
+}
+
 /*
  * Lock refname, without following symrefs, and set *lock_p to point
  * at a newly-allocated lock object. Fill in lock->old_oid, referent,
@@ -677,16 +697,17 @@ static void unlock_ref(struct ref_lock *lock)
  * - Generate informative error messages in the case of failure
  */
 static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs,
-					       struct ref_update *update,
+					       struct ref_transaction *transaction,
 					       size_t update_idx,
 					       int mustexist,
 					       struct string_list *refnames_to_check,
-					       const struct string_list *extras,
 					       struct ref_lock **lock_p,
 					       struct strbuf *referent,
 					       struct strbuf *err)
 {
 	enum ref_transaction_error ret = REF_TRANSACTION_ERROR_GENERIC;
+	struct ref_update *update = transaction->updates[update_idx];
+	const struct string_list *extras = &transaction->refnames;
 	const char *refname = update->refname;
 	unsigned int *type = &update->type;
 	struct ref_lock *lock;
@@ -776,6 +797,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 &&
+			    transaction_has_case_conflicting_update(transaction, update))
+				ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
 			goto error_return;
 		}
 	}
@@ -2583,9 +2607,8 @@ static enum ref_transaction_error lock_ref_for_update(struct files_ref_store *re
 	if (lock) {
 		lock->count++;
 	} else {
-		ret = lock_raw_ref(refs, update, update_idx, mustexist,
-				   refnames_to_check, &transaction->refnames,
-				   &lock, &referent, err);
+		ret = lock_raw_ref(refs, transaction, update_idx, mustexist,
+				   refnames_to_check, &lock, &referent, err);
 		if (ret) {
 			char *reason;
 
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 96648a6e5d..08d5df2af7 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -2294,6 +2294,59 @@ do
 		)
 	'
 
+	test_expect_success CASE_INSENSITIVE_FS,REFFILES "stdin $type batch-updates existing reference" '
+		git init repo &&
+		test_when_finished "rm -fr repo" &&
+		(
+			cd repo &&
+			test_commit one &&
+			old_head=$(git rev-parse HEAD) &&
+			test_commit two &&
+			head=$(git rev-parse HEAD) &&
+
+			{
+				format_command $type "create refs/heads/foo" "$head" &&
+				format_command $type "create refs/heads/ref" "$old_head" &&
+				format_command $type "create refs/heads/Foo" "$old_head"
+			} >stdin &&
+			git update-ref $type --stdin --batch-updates <stdin >stdout &&
+
+			echo $head >expect &&
+			git rev-parse refs/heads/foo >actual &&
+			echo $old_head >expect &&
+			git rev-parse refs/heads/ref >actual &&
+			test_cmp expect actual &&
+			test_grep -q "reference conflict due to case-insensitive filesystem" stdout
+		)
+	'
+
+	test_expect_success CASE_INSENSITIVE_FS "stdin $type batch-updates existing reference" '
+		git init --ref-format=reftable repo &&
+		test_when_finished "rm -fr repo" &&
+		(
+			cd repo &&
+			test_commit one &&
+			old_head=$(git rev-parse HEAD) &&
+			test_commit two &&
+			head=$(git rev-parse HEAD) &&
+
+			{
+				format_command $type "create refs/heads/foo" "$head" &&
+				format_command $type "create refs/heads/ref" "$old_head" &&
+				format_command $type "create refs/heads/Foo" "$old_head"
+			} >stdin &&
+			git update-ref $type --stdin --batch-updates <stdin >stdout &&
+
+			echo $head >expect &&
+			git rev-parse refs/heads/foo >actual &&
+			echo $old_head >expect &&
+			git rev-parse refs/heads/ref >actual &&
+			test_cmp expect actual &&
+			git rev-parse refs/heads/Foo >actual &&
+			test_cmp expect actual
+		)
+	'
+
 	test_expect_success "stdin $type batch-updates delete incorrect symbolic ref" '
 		git init repo &&
 		test_when_finished "rm -fr repo" &&
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index ebc696546b..57f60da81b 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -47,7 +47,13 @@ test_expect_success "clone and setup child repos" '
 		git config set branch.main.merge refs/heads/one
 	) &&
 	git clone . bundle &&
-	git clone . seven
+	git clone . seven &&
+	git clone --ref-format=reftable . case_sensitive &&
+	(
+		cd case_sensitive &&
+		git branch branch1 &&
+		git branch bRanch1
+	)
 '
 
 test_expect_success "fetch test" '
@@ -1526,6 +1532,20 @@ test_expect_success SYMLINKS 'clone does not get confused by a D/F conflict' '
 	test_path_is_missing whoops
 '
 
+test_expect_success CASE_INSENSITIVE_FS,REFFILES 'existing references in a case insensitive filesystem' '
+	test_when_finished rm -rf case_insensitive &&
+	(
+		git init --bare case_insensitive &&
+		cd case_insensitive &&
+		git remote add origin -- ../case_sensitive &&
+		test_must_fail git fetch -f origin "refs/heads/*:refs/heads/*" 2>err &&
+		test_grep "You${SQ}re on a case-insensitive filesystem" err &&
+		git rev-parse refs/heads/main >expect &&
+		git rev-parse refs/heads/branch1 >actual &&
+		test_cmp expect actual
+	)
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 

-- 
2.50.1

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

[Justin Tobler]

On 25/09/13 10:54PM, Karthik Nayak wrote:
> During the 'prepare' phase of reference transaction in the files

s/reference/a reference/

> backend, we create the lock files for references to be created. When
> using batched updates on case-insensitive filesystems, the entire
> batched updates would be aborted if there are conflicting names such as:
> 
>   refs/heads/Foo
>   refs/heads/foo

Ok so this is only a problem now because the reference updates are
performed in a single transaction and the resulting error causes the
entire transaction to be aborted.

> This affects all commands which were migrated to use batched updates in
> Git 2.51, including 'git-fetch(1)' and 'git-receive-pack(1)'. Before
> that, reference updates would be applied serially with one transaction
> used per update. When users fetched multiple references on
> case-insensitive systems, subsequent references would simply overwrite
> any earlier references. So when fetching:
> 
>   refs/heads/foo: 5f34ec0bfeac225b1c854340257a65b106f70ea6
>   refs/heads/Foo: ec3053b0977e83d9b67fc32c4527a117953994f3
>   refs/heads/sample: 2eefd1150e06d8fca1ddfa684dec016f36bf4e56
> 
> The user would simply end up with:
> 
>   refs/heads/foo: ec3053b0977e83d9b67fc32c4527a117953994f3
>   refs/heads/sample: 2eefd1150e06d8fca1ddfa684dec016f36bf4e56

Makes sense.

> This is buggy behavior since the user is never informed about the
> overrides performed and missing references. Nevertheless, the user is
> left with a working repository with a subset of the references. Since
> Git 2.51, in such situations fetches would simply fail without updating
> any references. Which is also buggy behavior and worse off since the
> user is left without any references.
> 
> The error is triggered in `lock_raw_ref()` where the files backend
> attempts to create a lock file. When a lock file already exists the
> function returns a 'REF_TRANSACTION_ERROR_GENERIC'. When this happens,
> the entire batched updates, not individual operation, is aborted as if
> it were in a transaction.
> 
> Change this to return 'REF_TRANSACTION_ERROR_CASE_CONFLICT' instead to
> aid the batched update mechanism to simply reject such errors. 

So does this mean that we return `REF_TRANSACTION_ERROR_CASE_CONFLICT`
in all cases where a a lockfile already exists for a reference? Or do we
only actually care about scenarios where the lockfile already exists
specific to case-insensitive filesystems?

> The
> change only affects batched updates since batched updates will reject
> individual updates with non-generic errors. So specifically this would
> only affect:
> 
>     1. git fetch
>     2. git receive-pack
>     3. git update-ref --batch-updates

Just to clarify, is this saying that this new error is not ignored in a
standard reference transaction? Only the above operations?

> This bubbles the error type up to `files_transaction_prepare()` which
> tries to lock each reference update. So if the locking fails, we check
> if the rejection type can be ignored, which is done by calling
> `ref_transaction_maybe_set_rejected()`.
> 
> As the error type is now 'REF_TRANSACTION_ERROR_CASE_CONFLICT',
> the specific reference update would simply be rejected, while other
> updates in the transaction would continue to be applied. This allows
> partial application of references in case-insensitive filesystems when
> fetching colliding references.
> 
> While the earlier implementation allowed the last reference to be
> applied overriding the initial references, this change would allow the
> first reference to be applied while rejecting consequent collisions.
> This should be an okay compromise since with the files backend, there is
> no scenario possible where we would retain all colliding references.
> 
> Let's also be more pro-active and notify users on case-insensitive

s/pro-active/proactive/

> filesystems about such problems by providing a brief about the issue
> while also recommending using the reftable backend, which doesn't have
> the same issue.
> 
> Reported-by: Joe Drew <joe.drew@indexexchange.com>
> Helped-by: Patrick Steinhardt <ps@pks.im>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  builtin/fetch.c       | 21 +++++++++++++++++---
>  refs.c                |  2 ++
>  refs.h                |  2 ++
>  refs/files-backend.c  | 33 +++++++++++++++++++++++++++-----
>  t/t1400-update-ref.sh | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  t/t5510-fetch.sh      | 22 ++++++++++++++++++++-
>  6 files changed, 124 insertions(+), 9 deletions(-)
> 
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 24645c4653..c7ff3480fb 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1643,7 +1643,8 @@ static int set_head(const struct ref *remote_refs, struct remote *remote)
>  
>  struct ref_rejection_data {
>  	int *retcode;
> -	int conflict_msg_shown;
> +	bool conflict_msg_shown;
> +	bool case_sensitive_msg_shown;
>  	const char *remote_name;
>  };
>  
> @@ -1657,11 +1658,25 @@ static void ref_transaction_rejection_handler(const char *refname,
>  {
>  	struct ref_rejection_data *data = cb_data;
>  
> -	if (err == REF_TRANSACTION_ERROR_NAME_CONFLICT && !data->conflict_msg_shown) {
> +	if (err == REF_TRANSACTION_ERROR_CASE_CONFLICT && ignore_case &&
> +	    !data->case_sensitive_msg_shown) {
> +		error(_("You're on a case-insensitive filesystem, and the remote you are\n"
> +			"trying to fetch from has references that only differ in casing. It\n"
> +			"is impossible to store such references with the 'files' backend. You\n"
> +			"can either accept this as-is, in which case you won't be able to\n"
> +			"store all remote references on disk. Or you can alternatively\n"
> +			"migrate your repository to use the 'reftable' backend with the\n"
> +			"following command:\n\n    git refs migrate --ref-format=reftable\n\n"
> +			"Please keep in mind that not all implementations of Git support this\n"
> +			"new format yet. So if you use tools other than Git to access this\n"
> +			"repository it may not be an option to migrate to reftables.\n"));

Nice error message.

[snip]
> +/*
> + * Check if the transaction has another update with a case-insensitive refname
> + * match.
> + *
> + * If the update is part of the transaction, we only check up to that index.
> + * Further updates are expected to call this function to match previous indices.
> + */
> +static bool transaction_has_case_conflicting_update(struct ref_transaction *transaction,
> +						    struct ref_update *update)
> +{
> +	for (size_t i = 0; i < transaction->nr; i++) {
> +		if (transaction->updates[i] == update)
> +			break;
> +
> +		if (!strcasecmp(transaction->updates[i]->refname, update->refname))
> +			return true;
> +	}
> +	return false;
> +}

Ah ok, so we do validate that the pre-existing lockfile comes from this
transaction. That is how we know it is related to case-insensitive
filesystems.

> +
>  /*
>   * Lock refname, without following symrefs, and set *lock_p to point
>   * at a newly-allocated lock object. Fill in lock->old_oid, referent,
> @@ -677,16 +697,17 @@ static void unlock_ref(struct ref_lock *lock)
>   * - Generate informative error messages in the case of failure
>   */
>  static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs,
> -					       struct ref_update *update,
> +					       struct ref_transaction *transaction,
>  					       size_t update_idx,
>  					       int mustexist,
>  					       struct string_list *refnames_to_check,
> -					       const struct string_list *extras,
>  					       struct ref_lock **lock_p,
>  					       struct strbuf *referent,
>  					       struct strbuf *err)
>  {
>  	enum ref_transaction_error ret = REF_TRANSACTION_ERROR_GENERIC;
> +	struct ref_update *update = transaction->updates[update_idx];
> +	const struct string_list *extras = &transaction->refnames;
>  	const char *refname = update->refname;
>  	unsigned int *type = &update->type;
>  	struct ref_lock *lock;
> @@ -776,6 +797,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);

huh, so if if we have a lockfile error due to a case-insensitve
filesystem, does this mean we print the error message from
`unable_to_lock_message()` and the new message?

If so, I wonder if we would be better off skipping the former since it
could be a bit misleading.

> +			if (myerr == EEXIST && ignore_case &&
> +			    transaction_has_case_conflicting_update(transaction, update))
> +				ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
>  			goto error_return;
>  		}
>  	}

-Justin

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

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 7830 bytes --]

Justin Tobler <jltobler@gmail.com> writes:

> On 25/09/13 10:54PM, Karthik Nayak wrote:
>> During the 'prepare' phase of reference transaction in the files
>
> s/reference/a reference/
>

I thought 'phase of reference transaction' without an 'a' is an accepted
form.. I'll add this locally for now.

>> backend, we create the lock files for references to be created. When
>> using batched updates on case-insensitive filesystems, the entire
>> batched updates would be aborted if there are conflicting names such as:
>>
>>   refs/heads/Foo
>>   refs/heads/foo
>
> Ok so this is only a problem now because the reference updates are
> performed in a single transaction and the resulting error causes the
> entire transaction to be aborted.
>

Yup, exactly. So conflicting references within the same transaction
become an issue since they try to lock the same file.

>> This affects all commands which were migrated to use batched updates in
>> Git 2.51, including 'git-fetch(1)' and 'git-receive-pack(1)'. Before
>> that, reference updates would be applied serially with one transaction
>> used per update. When users fetched multiple references on
>> case-insensitive systems, subsequent references would simply overwrite
>> any earlier references. So when fetching:
>>
>>   refs/heads/foo: 5f34ec0bfeac225b1c854340257a65b106f70ea6
>>   refs/heads/Foo: ec3053b0977e83d9b67fc32c4527a117953994f3
>>   refs/heads/sample: 2eefd1150e06d8fca1ddfa684dec016f36bf4e56
>>
>> The user would simply end up with:
>>
>>   refs/heads/foo: ec3053b0977e83d9b67fc32c4527a117953994f3
>>   refs/heads/sample: 2eefd1150e06d8fca1ddfa684dec016f36bf4e56
>
> Makes sense.
>
>> This is buggy behavior since the user is never informed about the
>> overrides performed and missing references. Nevertheless, the user is
>> left with a working repository with a subset of the references. Since
>> Git 2.51, in such situations fetches would simply fail without updating
>> any references. Which is also buggy behavior and worse off since the
>> user is left without any references.
>>
>> The error is triggered in `lock_raw_ref()` where the files backend
>> attempts to create a lock file. When a lock file already exists the
>> function returns a 'REF_TRANSACTION_ERROR_GENERIC'. When this happens,
>> the entire batched updates, not individual operation, is aborted as if
>> it were in a transaction.
>>
>> Change this to return 'REF_TRANSACTION_ERROR_CASE_CONFLICT' instead to
>> aid the batched update mechanism to simply reject such errors.
>
> So does this mean that we return `REF_TRANSACTION_ERROR_CASE_CONFLICT`
> in all cases where a a lockfile already exists for a reference? Or do we
> only actually care about scenarios where the lockfile already exists
> specific to case-insensitive filesystems?
>

This patch specifically only returns when there is a conflict in
case-insensitive systems.

>> The
>> change only affects batched updates since batched updates will reject
>> individual updates with non-generic errors. So specifically this would
>> only affect:
>>
>>     1. git fetch
>>     2. git receive-pack
>>     3. git update-ref --batch-updates
>
> Just to clarify, is this saying that this new error is not ignored in a
> standard reference transaction? Only the above operations?
>

Yes, only batched updates care about error types. Regular transactions
treat all errors the same.

>> This bubbles the error type up to `files_transaction_prepare()` which
>> tries to lock each reference update. So if the locking fails, we check
>> if the rejection type can be ignored, which is done by calling
>> `ref_transaction_maybe_set_rejected()`.
>>
>> As the error type is now 'REF_TRANSACTION_ERROR_CASE_CONFLICT',
>> the specific reference update would simply be rejected, while other
>> updates in the transaction would continue to be applied. This allows
>> partial application of references in case-insensitive filesystems when
>> fetching colliding references.
>>
>> While the earlier implementation allowed the last reference to be
>> applied overriding the initial references, this change would allow the
>> first reference to be applied while rejecting consequent collisions.
>> This should be an okay compromise since with the files backend, there is
>> no scenario possible where we would retain all colliding references.
>>
>> Let's also be more pro-active and notify users on case-insensitive
>
> s/pro-active/proactive/
>

Will change.

>> @@ -1657,11 +1658,25 @@ static void ref_transaction_rejection_handler(const char *refname,
>>  {
>>  	struct ref_rejection_data *data = cb_data;
>>
>> -	if (err == REF_TRANSACTION_ERROR_NAME_CONFLICT && !data->conflict_msg_shown) {
>> +	if (err == REF_TRANSACTION_ERROR_CASE_CONFLICT && ignore_case &&
>> +	    !data->case_sensitive_msg_shown) {
>> +		error(_("You're on a case-insensitive filesystem, and the remote you are\n"
>> +			"trying to fetch from has references that only differ in casing. It\n"
>> +			"is impossible to store such references with the 'files' backend. You\n"
>> +			"can either accept this as-is, in which case you won't be able to\n"
>> +			"store all remote references on disk. Or you can alternatively\n"
>> +			"migrate your repository to use the 'reftable' backend with the\n"
>> +			"following command:\n\n    git refs migrate --ref-format=reftable\n\n"
>> +			"Please keep in mind that not all implementations of Git support this\n"
>> +			"new format yet. So if you use tools other than Git to access this\n"
>> +			"repository it may not be an option to migrate to reftables.\n"));
>
> Nice error message.
>

All thanks to Patrick!

>> +
>>  /*
>>   * Lock refname, without following symrefs, and set *lock_p to point
>>   * at a newly-allocated lock object. Fill in lock->old_oid, referent,
>> @@ -677,16 +697,17 @@ static void unlock_ref(struct ref_lock *lock)
>>   * - Generate informative error messages in the case of failure
>>   */
>>  static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs,
>> -					       struct ref_update *update,
>> +					       struct ref_transaction *transaction,
>>  					       size_t update_idx,
>>  					       int mustexist,
>>  					       struct string_list *refnames_to_check,
>> -					       const struct string_list *extras,
>>  					       struct ref_lock **lock_p,
>>  					       struct strbuf *referent,
>>  					       struct strbuf *err)
>>  {
>>  	enum ref_transaction_error ret = REF_TRANSACTION_ERROR_GENERIC;
>> +	struct ref_update *update = transaction->updates[update_idx];
>> +	const struct string_list *extras = &transaction->refnames;
>>  	const char *refname = update->refname;
>>  	unsigned int *type = &update->type;
>>  	struct ref_lock *lock;
>> @@ -776,6 +797,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);
>
> huh, so if if we have a lockfile error due to a case-insensitve
> filesystem, does this mean we print the error message from
> `unable_to_lock_message()` and the new message?
>
> If so, I wonder if we would be better off skipping the former since it
> could be a bit misleading.
>

I would say both are necessary. The errors added here are more technical
and really talk about why we faced an issue. The error in
'builtin/fetch.c' is more about guidance to how to overcome that issue.

Also this error is client agnostic, so we'd add the error here for users
of both regular transactions and batched updates. The error in
'builtin/fetch.c' is very specific to users of 'git-fetch(1)'. So I
think both hold value.

>> +			if (myerr == EEXIST && ignore_case &&
>> +			    transaction_has_case_conflicting_update(transaction, update))
>> +				ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
>>  			goto error_return;
>>  		}
>>  	}
>
> -Justin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

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

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 4146 bytes --]

Karthik Nayak <karthik.188@gmail.com> writes:

>>> +
>>>  /*
>>>   * Lock refname, without following symrefs, and set *lock_p to point
>>>   * at a newly-allocated lock object. Fill in lock->old_oid, referent,
>>> @@ -677,16 +697,17 @@ static void unlock_ref(struct ref_lock *lock)
>>>   * - Generate informative error messages in the case of failure
>>>   */
>>>  static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs,
>>> -					       struct ref_update *update,
>>> +					       struct ref_transaction *transaction,
>>>  					       size_t update_idx,
>>>  					       int mustexist,
>>>  					       struct string_list *refnames_to_check,
>>> -					       const struct string_list *extras,
>>>  					       struct ref_lock **lock_p,
>>>  					       struct strbuf *referent,
>>>  					       struct strbuf *err)
>>>  {
>>>  	enum ref_transaction_error ret = REF_TRANSACTION_ERROR_GENERIC;
>>> +	struct ref_update *update = transaction->updates[update_idx];
>>> +	const struct string_list *extras = &transaction->refnames;
>>>  	const char *refname = update->refname;
>>>  	unsigned int *type = &update->type;
>>>  	struct ref_lock *lock;
>>> @@ -776,6 +797,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);
>>
>> huh, so if if we have a lockfile error due to a case-insensitve
>> filesystem, does this mean we print the error message from
>> `unable_to_lock_message()` and the new message?
>>
>> If so, I wonder if we would be better off skipping the former since it
>> could be a bit misleading.
>>
>
> I would say both are necessary. The errors added here are more technical
> and really talk about why we faced an issue. The error in
> 'builtin/fetch.c' is more about guidance to how to overcome that issue.
>
> Also this error is client agnostic, so we'd add the error here for users
> of both regular transactions and batched updates. The error in
> 'builtin/fetch.c' is very specific to users of 'git-fetch(1)'. So I
> think both hold value.
>

My response was a bit confusing here. To answer your question directly.
No we do not print both messages. The error message is appended to the
`strbuf err` as you see.

This 'strbuf' is provided by the callee, specifically in 'git-fetch(1)',
we only print this error message if the ref transaction failed. Since
'git-fetch(1)' uses batched updates, the transaction will only fail when
we encounter a GENERIC error type. Otherwise, the transaction succeeds
with/without rejected updates. After which 'git-fetch(1)' will print out
specific messages for rejected updates.

For example, with the test added in this patch:

  $ ~/code/git/build/bin-wrappers/git fetch -f origin
"refs/heads/*:refs/heads/*"
  remote: Enumerating objects: 3, done.
  remote: Counting objects: 100% (3/3), done.
  remote: Total 3 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0)
  Unpacking objects: 100% (3/3), 713 bytes | 713.00 KiB/s, done.
  From ../base
   * [new branch]      bRanch1    -> bRanch1
   * [new branch]      branch1    -> branch1
   * [new branch]      master     -> master
   * [new branch]      bRanch1    -> origin/bRanch1
   * [new branch]      branch1    -> origin/branch1
   * [new branch]      master     -> origin/master
  error: You're on a case-insensitive filesystem, and the remote you are
  trying to fetch from has references that only differ in casing. It
  is impossible to store such references with the 'files' backend. You
  can either accept this as-is, in which case you won't be able to
  store all remote references on disk. Or you can alternatively
  migrate your repository to use the 'reftable' backend with the
  following command:

      git refs migrate --ref-format=reftable

  Please keep in mind that not all implementations of Git support this
  new format yet. So if you use tools other than Git to access this
  repository it may not be an option to migrate to reftables.

  error: fetching ref refs/remotes/origin/branch1 failed: reference
conflict due to case-insensitive filesystem

Hope that clarifies it!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

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

[Justin Tobler]

On 25/09/17 12:33AM, Karthik Nayak wrote:
> Justin Tobler <jltobler@gmail.com> writes:
> > On 25/09/13 10:54PM, Karthik Nayak wrote:
> >> +
> >>  /*
> >>   * Lock refname, without following symrefs, and set *lock_p to point
> >>   * at a newly-allocated lock object. Fill in lock->old_oid, referent,
> >> @@ -677,16 +697,17 @@ static void unlock_ref(struct ref_lock *lock)
> >>   * - Generate informative error messages in the case of failure
> >>   */
> >>  static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs,
> >> -					       struct ref_update *update,
> >> +					       struct ref_transaction *transaction,
> >>  					       size_t update_idx,
> >>  					       int mustexist,
> >>  					       struct string_list *refnames_to_check,
> >> -					       const struct string_list *extras,
> >>  					       struct ref_lock **lock_p,
> >>  					       struct strbuf *referent,
> >>  					       struct strbuf *err)
> >>  {
> >>  	enum ref_transaction_error ret = REF_TRANSACTION_ERROR_GENERIC;
> >> +	struct ref_update *update = transaction->updates[update_idx];
> >> +	const struct string_list *extras = &transaction->refnames;
> >>  	const char *refname = update->refname;
> >>  	unsigned int *type = &update->type;
> >>  	struct ref_lock *lock;
> >> @@ -776,6 +797,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);
> >
> > huh, so if if we have a lockfile error due to a case-insensitve
> > filesystem, does this mean we print the error message from
> > `unable_to_lock_message()` and the new message?
> >
> > If so, I wonder if we would be better off skipping the former since it
> > could be a bit misleading.
> >
> 
> I would say both are necessary. The errors added here are more technical
> and really talk about why we faced an issue. The error in
> 'builtin/fetch.c' is more about guidance to how to overcome that issue.
> 
> Also this error is client agnostic, so we'd add the error here for users
> of both regular transactions and batched updates. The error in
> 'builtin/fetch.c' is very specific to users of 'git-fetch(1)'. So I
> think both hold value.

Ah ok, I was orginally concerned that both error messages would be
printed when a reference update is rejected due to the case conflict
error. I see now that the "Unable to create '%s.lock': %s." message only
gets printed if the transaction actually fails. With this change, a case
conflict error on an individual references in batched updates doesn't
result in the entire transaction failing. Thus, only the new message is
printed. Makes sense.

Thanks,
-Justin

[PATCH v3 2/4] refs/files: use correct error type when lock exists

[Karthik Nayak]

When fetching references into a repository, if a lock for a particular
reference exists, then `lock_raw_ref()` throws:

    - REF_TRANSACTION_ERROR_CASE_CONFLICT: when there is a conflict
    because transaction contains conflicting references while being on a
    case-insensitive filesystem.

    - REF_TRANSACTION_ERROR_GENERIC: for all other errors.

The latter causes the entire set of batched updates to fail, even in
case sensitive filessystems.

Instead, return a 'REF_TRANSACTION_ERROR_CREATE_EXISTS' error. This
allows batched updates to reject the individual update which conflicts
with the existing file, while updating the rest of the references.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs/files-backend.c | 20 +++++++++++++++++---
 t/t5510-fetch.sh     | 26 ++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 01df32904b..69e50a16db 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -797,9 +797,23 @@ 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 &&
-			    transaction_has_case_conflicting_update(transaction, update))
-				ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
+			if (myerr == EEXIST) {
+				if (ignore_case &&
+				    transaction_has_case_conflicting_update(transaction, update))
+					/*
+					 * In case-insensitive filesystems, ensure that conflicts within a
+					 * given transaction are handled. Pre-existing refs on a
+					 * case-insensitive system will be overridden without any issue.
+					 */
+					ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
+				else
+					/*
+					 * Pre-existing case-conflicting reference locks should also be
+					 * specially categorized to avoid failing all batched updates.
+					 */
+					ret = REF_TRANSACTION_ERROR_CREATE_EXISTS;
+			}
+
 			goto error_return;
 		}
 	}
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 57f60da81b..6f8db0ace4 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -1546,6 +1546,32 @@ test_expect_success CASE_INSENSITIVE_FS,REFFILES 'existing references in a case
 	)
 '
 
+test_expect_success REFFILES 'existing reference lock in repo' '
+	test_when_finished rm -rf base repo &&
+	(
+		git init --ref-format=reftable base &&
+		cd base &&
+		echo >file update &&
+		git add . &&
+		git commit -m "updated" &&
+		git branch -M main &&
+
+		git update-ref refs/heads/foo @ &&
+		git update-ref refs/heads/branch @ &&
+		cd .. &&
+
+		git init --ref-format=files --bare repo &&
+		cd repo &&
+		git remote add origin ../base &&
+		touch refs/heads/foo.lock &&
+		test_must_fail git fetch -f origin "refs/heads/*:refs/heads/*" 2>err &&
+		test_grep "error: fetching ref refs/heads/foo failed: reference already exists" err &&
+		git rev-parse refs/heads/main >expect &&
+		git rev-parse refs/heads/branch >actual &&
+		test_cmp expect actual
+	)
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 

-- 
2.50.1

Re: [PATCH v3 2/4] refs/files: use correct error type when lock exists

[Patrick Steinhardt]

On Sat, Sep 13, 2025 at 10:54:30PM +0200, Karthik Nayak wrote:
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 01df32904b..69e50a16db 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -797,9 +797,23 @@ 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 &&
> -			    transaction_has_case_conflicting_update(transaction, update))
> -				ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
> +			if (myerr == EEXIST) {
> +				if (ignore_case &&
> +				    transaction_has_case_conflicting_update(transaction, update))
> +					/*
> +					 * In case-insensitive filesystems, ensure that conflicts within a
> +					 * given transaction are handled. Pre-existing refs on a
> +					 * case-insensitive system will be overridden without any issue.
> +					 */
> +					ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
> +				else
> +					/*
> +					 * Pre-existing case-conflicting reference locks should also be
> +					 * specially categorized to avoid failing all batched updates.
> +					 */
> +					ret = REF_TRANSACTION_ERROR_CREATE_EXISTS;
> +			}

Tiniest nit: I think it would be preferable to use curly braces for such
multi-line comments. This nit isn't worth a reroll though.

Other than that my feedback got addressed, so this looks good to me.
Thanks!

Patrick

Re: [PATCH v3 2/4] refs/files: use correct error type when lock exists

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 1687 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> On Sat, Sep 13, 2025 at 10:54:30PM +0200, Karthik Nayak wrote:
>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index 01df32904b..69e50a16db 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -797,9 +797,23 @@ 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 &&
>> -			    transaction_has_case_conflicting_update(transaction, update))
>> -				ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
>> +			if (myerr == EEXIST) {
>> +				if (ignore_case &&
>> +				    transaction_has_case_conflicting_update(transaction, update))
>> +					/*
>> +					 * In case-insensitive filesystems, ensure that conflicts within a
>> +					 * given transaction are handled. Pre-existing refs on a
>> +					 * case-insensitive system will be overridden without any issue.
>> +					 */
>> +					ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
>> +				else
>> +					/*
>> +					 * Pre-existing case-conflicting reference locks should also be
>> +					 * specially categorized to avoid failing all batched updates.
>> +					 */
>> +					ret = REF_TRANSACTION_ERROR_CREATE_EXISTS;
>> +			}
>
> Tiniest nit: I think it would be preferable to use curly braces for such
> multi-line comments. This nit isn't worth a reroll though.
>

I did contemplate about this in my mind, thanks for raising this. I will
change it locally for now and push a new version if needed.

> Other than that my feedback got addressed, so this looks good to me.
> Thanks!
>
> Patrick

Thanks for your reviews!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH v3 2/4] refs/files: use correct error type when lock exists

[Justin Tobler]

On 25/09/13 10:54PM, Karthik Nayak wrote:
> When fetching references into a repository, if a lock for a particular
> reference exists, then `lock_raw_ref()` throws:
> 
>     - REF_TRANSACTION_ERROR_CASE_CONFLICT: when there is a conflict
>     because transaction contains conflicting references while being on a

s/transaction/the transaction/

>     case-insensitive filesystem.
> 
>     - REF_TRANSACTION_ERROR_GENERIC: for all other errors.
> 
> The latter causes the entire set of batched updates to fail, even in
> case sensitive filessystems.

Ok so this issue isn't related to case-insensitive filesystems. The
issue is that now we use batch updated, a single pre-existing lockfile
causes the entire transaction to fail. Prior to batch updates, only the
individual update would fail, but wouldn't stop others.

> Instead, return a 'REF_TRANSACTION_ERROR_CREATE_EXISTS' error. This
> allows batched updates to reject the individual update which conflicts
> with the existing file, while updating the rest of the references.

Make sense.

> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  refs/files-backend.c | 20 +++++++++++++++++---
>  t/t5510-fetch.sh     | 26 ++++++++++++++++++++++++++
>  2 files changed, 43 insertions(+), 3 deletions(-)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 01df32904b..69e50a16db 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -797,9 +797,23 @@ 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 &&
> -			    transaction_has_case_conflicting_update(transaction, update))
> -				ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
> +			if (myerr == EEXIST) {
> +				if (ignore_case &&
> +				    transaction_has_case_conflicting_update(transaction, update))
> +					/*
> +					 * In case-insensitive filesystems, ensure that conflicts within a
> +					 * given transaction are handled. Pre-existing refs on a
> +					 * case-insensitive system will be overridden without any issue.
> +					 */
> +					ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
> +				else
> +					/*
> +					 * Pre-existing case-conflicting reference locks should also be
> +					 * specially categorized to avoid failing all batched updates.
> +					 */
> +					ret = REF_TRANSACTION_ERROR_CREATE_EXISTS;

IIUC, by returning a non-generic error here the individual reference
will be rejected during batch updates instead of aborting the
transaction.

-Justin

Re: [PATCH v3 2/4] refs/files: use correct error type when lock exists

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 2951 bytes --]

Justin Tobler <jltobler@gmail.com> writes:

> On 25/09/13 10:54PM, Karthik Nayak wrote:
>> When fetching references into a repository, if a lock for a particular
>> reference exists, then `lock_raw_ref()` throws:
>>
>>     - REF_TRANSACTION_ERROR_CASE_CONFLICT: when there is a conflict
>>     because transaction contains conflicting references while being on a
>
> s/transaction/the transaction/
>

Makes sense.

>>     case-insensitive filesystem.
>>
>>     - REF_TRANSACTION_ERROR_GENERIC: for all other errors.
>>
>> The latter causes the entire set of batched updates to fail, even in
>> case sensitive filessystems.
>
> Ok so this issue isn't related to case-insensitive filesystems. The
> issue is that now we use batch updated, a single pre-existing lockfile
> causes the entire transaction to fail. Prior to batch updates, only the
> individual update would fail, but wouldn't stop others.
>
>> Instead, return a 'REF_TRANSACTION_ERROR_CREATE_EXISTS' error. This
>> allows batched updates to reject the individual update which conflicts
>> with the existing file, while updating the rest of the references.
>
> Make sense.
>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>>  refs/files-backend.c | 20 +++++++++++++++++---
>>  t/t5510-fetch.sh     | 26 ++++++++++++++++++++++++++
>>  2 files changed, 43 insertions(+), 3 deletions(-)
>>
>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index 01df32904b..69e50a16db 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -797,9 +797,23 @@ 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 &&
>> -			    transaction_has_case_conflicting_update(transaction, update))
>> -				ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
>> +			if (myerr == EEXIST) {
>> +				if (ignore_case &&
>> +				    transaction_has_case_conflicting_update(transaction, update))
>> +					/*
>> +					 * In case-insensitive filesystems, ensure that conflicts within a
>> +					 * given transaction are handled. Pre-existing refs on a
>> +					 * case-insensitive system will be overridden without any issue.
>> +					 */
>> +					ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
>> +				else
>> +					/*
>> +					 * Pre-existing case-conflicting reference locks should also be
>> +					 * specially categorized to avoid failing all batched updates.
>> +					 */
>> +					ret = REF_TRANSACTION_ERROR_CREATE_EXISTS;
>
> IIUC, by returning a non-generic error here the individual reference
> will be rejected during batch updates instead of aborting the
> transaction.
>
> -Justin

Indeed. Batched updates allow rejecting of individual updates. It only
aborts when faced with a GENERIC error. So by marking this as
non-generic, it doesn't change the flow for regular transactions but
only how batched updates would handle it.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

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

[Karthik Nayak]

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
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.

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));
+		} else {
+			item = string_list_append(refnames_to_check, refname);
+		}
+
 		item->util = xmalloc(sizeof(update_idx));
 		memcpy(item->util, &update_idx, sizeof(update_idx));
 	}
@@ -2831,7 +2846,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 6f8db0ace4..08dbea6503 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
 	)
 '
 
@@ -1572,6 +1578,20 @@ test_expect_success REFFILES 'existing reference lock in repo' '
 	)
 '
 
+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
 

-- 
2.50.1

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

[Justin Tobler]

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

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

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 3434 bytes --]

Justin Tobler <jltobler@gmail.com> writes:

> 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/
>

Will change. Thanks.

>> 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.
>

Yup. That's correct!

>> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

[PATCH v3 4/4] refs/files: handle D/F conflicts during locking

[Karthik Nayak]

The previous commit added the necessary validation and checks for F/D
conflicts in the files backend when working on case insensitive systems.

There is still a possibility for D/F conflicts. This is a different from
the F/D since for F/D conflicts, there would not be a conflict during
the lock creation phase:

    refs/heads/foo.lock
    refs/heads/foo/bar.lock

However there would be a conflict when the locks are committed, since we
cannot have 'refs/heads/foo/bar' and 'refs/heads/foo'. These kinds of
conflicts are checked and resolved in
`refs_verify_refnames_available()`, so the previous commit ensured that
for case-insensitive filesystems, we would lowercase the inputs to that
function.

For D/F conflicts, there is a conflict during the lock creation phase
itself:

    refs/heads/foo/bar.lock
    refs/heads/foo.lock

As in `lock_raw_ref()` after creating the lock, we also check for D/F
conflicts. This can occur in case-insensitive filesystems when trying to
fetch case-conflicted references like:

    refs/heads/Foo/new
    refs/heads/foo

D/F conflicts can also occur in case-sensitive filesystems, when the
repository already contains a directory with a lock file
'refs/heads/foo/bar.lock' and trying to fetch 'refs/heads/foo'. This
doesn't concern directories containing garbage files as those are
handled on a higher level.

To fix this, simply categorize the error as a name conflict. Also remove
this reference from the list of valid refnames for availability checks.
By categorizing the error and removing it from the list of valid
references, batched updates now knows to reject such reference updates
and apply the other reference updates.

Fix a small typo in `ref_transaction_maybe_set_rejected()` while here.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs.c               |  9 ++++++++-
 refs/files-backend.c | 11 ++++++-----
 t/t5510-fetch.sh     | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 60 insertions(+), 6 deletions(-)

diff --git a/refs.c b/refs.c
index 4c1c339ed9..e7109ea5fe 100644
--- a/refs.c
+++ b/refs.c
@@ -1223,7 +1223,7 @@ int ref_transaction_maybe_set_rejected(struct ref_transaction *transaction,
 		return 0;
 
 	if (!transaction->rejections)
-		BUG("transaction not inititalized with failure support");
+		BUG("transaction not initialized with failure support");
 
 	/*
 	 * Don't accept generic errors, since these errors are not user
@@ -1232,6 +1232,13 @@ int ref_transaction_maybe_set_rejected(struct ref_transaction *transaction,
 	if (err == REF_TRANSACTION_ERROR_GENERIC)
 		return 0;
 
+	/*
+	 * Rejected refnames shouldn't be considered in the availability
+	 * checks, so remove them from the list.
+	 */
+	string_list_remove(&transaction->refnames,
+			   transaction->updates[update_idx]->refname, 0);
+
 	transaction->updates[update_idx]->rejection_err = err;
 	ALLOC_GROW(transaction->rejections->update_indices,
 		   transaction->rejections->nr + 1,
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 817b56f4ce..fec7713ea6 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -869,6 +869,7 @@ static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs,
 				goto error_return;
 			} else if (remove_dir_recursively(&ref_file,
 							  REMOVE_DIR_EMPTY_ONLY)) {
+				ret = REF_TRANSACTION_ERROR_NAME_CONFLICT;
 				if (refs_verify_refname_available(
 						    &refs->base, refname,
 						    extras, NULL, 0, err)) {
@@ -876,14 +877,14 @@ static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs,
 					 * The error message set by
 					 * verify_refname_available() is OK.
 					 */
-					ret = REF_TRANSACTION_ERROR_NAME_CONFLICT;
 					goto error_return;
 				} else {
 					/*
-					 * We can't delete the directory,
-					 * but we also don't know of any
-					 * references that it should
-					 * contain.
+					 * Directory conflicts can occur if there
+					 * is an existing lock file in the directory
+					 * or if the filesystem is case-insensitive
+					 * and the directory contains a valid reference
+					 * but conflicts with the update.
 					 */
 					strbuf_addf(err, "there is a non-empty directory '%s' "
 						    "blocking reference '%s'",
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 08dbea6503..6b2739db26 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -59,6 +59,12 @@ test_expect_success "clone and setup child repos" '
 		cd case_sensitive_fd &&
 		git branch foo/bar &&
 		git branch Foo
+	) &&
+	git clone --ref-format=reftable . case_sensitive_df &&
+	(
+		cd case_sensitive_df &&
+		git branch Foo/bar &&
+		git branch foo
 	)
 '
 
@@ -1592,6 +1598,46 @@ test_expect_success CASE_INSENSITIVE_FS,REFFILES 'F/D conflict on case insensiti
 	)
 '
 
+test_expect_success CASE_INSENSITIVE_FS,REFFILES 'D/F 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_df &&
+		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_expect_success REFFILES 'D/F conflict on case sensitive filesystem with lock' '
+	(
+		git init --ref-format=reftable base &&
+		cd base &&
+		echo >file update &&
+		git add . &&
+		git commit -m "updated" &&
+		git branch -M main &&
+
+		git update-ref refs/heads/foo @ &&
+		git update-ref refs/heads/branch @ &&
+		cd .. &&
+
+		git init --ref-format=files --bare repo &&
+		cd repo &&
+		git remote add origin ../base &&
+		mkdir refs/heads/foo &&
+		touch refs/heads/foo/random.lock &&
+		test_must_fail git fetch origin "refs/heads/*:refs/heads/*" 2>err &&
+		test_grep "some local refs could not be updated; try running" err &&
+		git rev-parse refs/heads/main >expect &&
+		git rev-parse refs/heads/branch >actual &&
+		test_cmp expect actual
+	)
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 

-- 
2.50.1

Re: [PATCH v3 0/4] refs/files: fix issues with git-fetch on case-insensitive FS

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> Changes in v3:
> - Rename duplicate_reference_case_cmp() to
>   transaction_has_case_conflicting_update() and add comments.
> - Improve commit messages.
> - Add an additional test in the 4th commit to showcase D/F conflicts in
>   case-sensistive file systems.
> - Link to v2: https://lore.kernel.org/r/20250908-587-git-fetch-1-fails-fetches-on-case-insensitive-repositories-v2-0-b2eb2459befb@gmail.com

I think I like this "latest first and then historical" order in the
cover letter much better than the other way around.

I see that this topic is pretty much done?  There still are a few
questions from Justin's reply that may want to be answered, but I
have a feeling that the answer to them would not require a new
iteration.

Looking good.  Thanks.

Re: [PATCH v3 0/4] refs/files: fix issues with git-fetch on case-insensitive FS

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 1298 bytes --]

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Changes in v3:
>> - Rename duplicate_reference_case_cmp() to
>>   transaction_has_case_conflicting_update() and add comments.
>> - Improve commit messages.
>> - Add an additional test in the 4th commit to showcase D/F conflicts in
>>   case-sensistive file systems.
>> - Link to v2: https://lore.kernel.org/r/20250908-587-git-fetch-1-fails-fetches-on-case-insensitive-repositories-v2-0-b2eb2459befb@gmail.com
>
> I think I like this "latest first and then historical" order in the
> cover letter much better than the other way around.
>
> I see that this topic is pretty much done?  There still are a few
> questions from Justin's reply that may want to be answered, but I
> have a feeling that the answer to them would not require a new
> iteration.
>
> Looking good.  Thanks.

I did respond to Justin, I think there were a few small nits around
grammar in the commit messages and a question around error reporting.

I have applied the grammar fixes locally and don't think they warrant a
re-roll. Regarding the error reporting. I think it is good the way it
is.

So I would say this is good as is and I will refrain for sending in
a new version, unless there is some other concern.

Thanks,
Karthik

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH v3 0/4] refs/files: fix issues with git-fetch on case-insensitive FS

[Toon Claes]

Karthik Nayak <karthik.188@gmail.com> writes:

> I did respond to Justin, I think there were a few small nits around
> grammar in the commit messages and a question around error reporting.
>
> I have applied the grammar fixes locally and don't think they warrant a
> re-roll. Regarding the error reporting. I think it is good the way it
> is.
>
> So I would say this is good as is and I will refrain for sending in
> a new version, unless there is some other concern.

I also gave it a round of review, and I agree it's good to go.

I've noticed another issue though, at the moment it's possible to create
a ref like `refs/heads/foo.Lock`. I can image this gives issue when the
remote has `refs/heads/foo` and `refs/heads/foo.Lock` and you pull those
in on a case-insensitive FS. Unfortunately I wasn't able to verify this.
But anyhow, I don't think that any reason to hold back on this current
patch series. I approve.

-- Toon

Re: [PATCH v3 0/4] refs/files: fix issues with git-fetch on case-insensitive FS

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 1506 bytes --]

Toon Claes <toon@iotcl.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> I did respond to Justin, I think there were a few small nits around
>> grammar in the commit messages and a question around error reporting.
>>
>> I have applied the grammar fixes locally and don't think they warrant a
>> re-roll. Regarding the error reporting. I think it is good the way it
>> is.
>>
>> So I would say this is good as is and I will refrain for sending in
>> a new version, unless there is some other concern.
>
> I also gave it a round of review, and I agree it's good to go.
>

Thanks, appreciate the review!

> I've noticed another issue though, at the moment it's possible to create
> a ref like `refs/heads/foo.Lock`. I can image this gives issue when the
> remote has `refs/heads/foo` and `refs/heads/foo.Lock` and you pull those
> in on a case-insensitive FS. Unfortunately I wasn't able to verify this.
> But anyhow, I don't think that any reason to hold back on this current
> patch series. I approve.

We don't fetch locks from remote. The locking mechanism is simply a
construct used to update files locally in race-free manner. Locking a
file ensures no other concurrent writes can happen.

In-short when you fetch references, the prepare stage of the reference
transaction will create the necessary lock files. This locks in the
updates with guarantee that no other process can update/create the refs.
The commit phase simply removes the lock files post updating the refs.

>
> -- Toon

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH v3 0/4] refs/files: fix issues with git-fetch on case-insensitive FS

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

>> I've noticed another issue though, at the moment it's possible to create
>> a ref like `refs/heads/foo.Lock`. I can image this gives issue when the
>> remote has `refs/heads/foo` and `refs/heads/foo.Lock` and you pull those
>> in on a case-insensitive FS. Unfortunately I wasn't able to verify this.
>> But anyhow, I don't think that any reason to hold back on this current
>> patch series. I approve.
>
> We don't fetch locks from remote. The locking mechanism is simply a
> construct used to update files locally in race-free manner. Locking a
> file ensures no other concurrent writes can happen.

I am not Toon, but I think Toon meant "foo.Lock" to be a funnily
named but a valid branch the remote has.

Doesn't the remote advertise refs/heads/foo and refs/heads/foo.Lock
in such a case?  And we can ask for both of them.  When updating
foo, we would locally create "refs/heads/foo.lock" and then rename
it to refs/heads/foo", right?  I think Toon's point was what happens
when the fetch of "foo.Lock" from there somehow has completed first.
Or we can simply fetch foo.Lock branch and then foo branch in a
separate invocation of "git fetch"---now wouldn't the second
invocation have trouble creating the lock file foo.lock for foo on
certain filesystems?

> In-short when you fetch references, the prepare stage of the reference
> transaction will create the necessary lock files. This locks in the
> updates with guarantee that no other process can update/create the refs.
> The commit phase simply removes the lock files post updating the refs.

Re: [PATCH v3 0/4] refs/files: fix issues with git-fetch on case-insensitive FS

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 3027 bytes --]

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>>> I've noticed another issue though, at the moment it's possible to create
>>> a ref like `refs/heads/foo.Lock`. I can image this gives issue when the
>>> remote has `refs/heads/foo` and `refs/heads/foo.Lock` and you pull those
>>> in on a case-insensitive FS. Unfortunately I wasn't able to verify this.
>>> But anyhow, I don't think that any reason to hold back on this current
>>> patch series. I approve.
>>
>> We don't fetch locks from remote. The locking mechanism is simply a
>> construct used to update files locally in race-free manner. Locking a
>> file ensures no other concurrent writes can happen.
>
> I am not Toon, but I think Toon meant "foo.Lock" to be a funnily
> named but a valid branch the remote has.
>

Oops. Thanks for explaining.

> Doesn't the remote advertise refs/heads/foo and refs/heads/foo.Lock
> in such a case?  And we can ask for both of them.  When updating
> foo, we would locally create "refs/heads/foo.lock" and then rename
> it to refs/heads/foo", right?

Yeah with batched updates, it would

1. create 'refs/heads/foo.lock'
2. before creating 'ref/heads/foo.Lock.lock', it would try to verify if
  'refs/heads/foo.Lock' already exists. Which it does from #1, so not
  create it at all.

To be honest, I'm not sure what the best way to solve this would be for
case-insensitive filesystems using the files backend.

> I think Toon's point was what happens
> when the fetch of "foo.Lock" from there somehow has completed first.

That shouldn't happen AFAIK since the reference updates are ordered
lexicographically.

> Or we can simply fetch foo.Lock branch and then foo branch in a
> separate invocation of "git fetch"---now wouldn't the second
> invocation have trouble creating the lock file foo.lock for foo on
> certain filesystems?

So if we:

1. fetch 'refs/heads/foo.Lock' to create 'refs/heads/foo.Lock' locally
2. fetch 'refs/heads/foo' after

#2 would fail since it would determine that 'refs/heads/foo.lock'
already exists. This would be the same behavior without batched updates
too though.

---

Overall, I think the only issue left would be when you try to fetch
'refs/heads/foo' and 'refs/heads/foo.Lock' together on a
case-insensitive filesystem with files backend. For such a system, with
batched updates, the latter reference wouldn't be created.

Thinking more about it, without batched updates, both would be created,
since we create the locks individually in separate transactions. But
after the first fetch, we would never be able to update 'refs/heads/foo'
since it would always think that a lock file for that reference exists.
So it was always broken in some sense. Sigh!

>> In-short when you fetch references, the prepare stage of the reference
>> transaction will create the necessary lock files. This locks in the
>> updates with guarantee that no other process can update/create the refs.
>> The commit phase simply removes the lock files post updating the refs.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH v3 0/4] refs/files: fix issues with git-fetch on case-insensitive FS

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> Changes in v3:
>>> - Rename duplicate_reference_case_cmp() to
>>>   transaction_has_case_conflicting_update() and add comments.
>>> - Improve commit messages.
>>> - Add an additional test in the 4th commit to showcase D/F conflicts in
>>>   case-sensistive file systems.
>>> - Link to v2: https://lore.kernel.org/r/20250908-587-git-fetch-1-fails-fetches-on-case-insensitive-repositories-v2-0-b2eb2459befb@gmail.com
>>
>> I think I like this "latest first and then historical" order in the
>> cover letter much better than the other way around.
>>
>> I see that this topic is pretty much done?  There still are a few
>> questions from Justin's reply that may want to be answered, but I
>> have a feeling that the answer to them would not require a new
>> iteration.
>>
>> Looking good.  Thanks.
>
> I did respond to Justin, I think there were a few small nits around
> grammar in the commit messages and a question around error reporting.

Well, if it is already locally ready, let's have a quick "small and
final" update before merging it to 'next'.

Thanks.

Re: [PATCH v3 0/4] refs/files: fix issues with git-fetch on case-insensitive FS

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 1331 bytes --]

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>
>>>> Changes in v3:
>>>> - Rename duplicate_reference_case_cmp() to
>>>>   transaction_has_case_conflicting_update() and add comments.
>>>> - Improve commit messages.
>>>> - Add an additional test in the 4th commit to showcase D/F conflicts in
>>>>   case-sensistive file systems.
>>>> - Link to v2: https://lore.kernel.org/r/20250908-587-git-fetch-1-fails-fetches-on-case-insensitive-repositories-v2-0-b2eb2459befb@gmail.com
>>>
>>> I think I like this "latest first and then historical" order in the
>>> cover letter much better than the other way around.
>>>
>>> I see that this topic is pretty much done?  There still are a few
>>> questions from Justin's reply that may want to be answered, but I
>>> have a feeling that the answer to them would not require a new
>>> iteration.
>>>
>>> Looking good.  Thanks.
>>
>> I did respond to Justin, I think there were a few small nits around
>> grammar in the commit messages and a question around error reporting.
>
> Well, if it is already locally ready, let's have a quick "small and
> final" update before merging it to 'next'.
>
> Thanks.

Surely, I've sent in v4.

Thanks,
Karthik

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

[PATCH v4 0/4] refs/files: fix issues with git-fetch on case-insensitive FS

[Karthik Nayak]

Hello!

With Git 2.51 we moved 'git-fetch(1)' and 'git-receive-pack(1)' to use
batched updates while doing reference updates. This provided a nice perf
boost since both commands will now use a single transaction for
reference updates. This removes the overhead of using individual
transaction per reference update and also avoids unnecessary
auto-compaction between reference updates in the reftable backend.

However, in the files-backend it does introduce a few bugs around
conflicts. The reported bug was around case-insensitive filesystems [1],
but we also fix some adjacent issues:

1. When fetching references such as:

   - refs/heads/foo
   - refs/heads/Foo

Earlier we would simply overwrite the first reference with the second
and continue. Since Git 2.51 we simply abort stating a conflict.

This is resolved in the first commit by explicitly categorizing the
error as non-GENERIC. This allows batched updates to reject the
particular update, while updating the rest.

2. When fetching references and a lock for a particular reference
already exits. We treat this is a GENERIC error, which fails the entire
update. By categorizing this error as non-GENERIC, we can reject this
specific update and update the other references.

3. When fetching references such as with F/D conflict:

  - refs/heads/foo
  - refs/heads/Foo/bar

Earlier we would apply the first, while the second would fail due to
conflict. Since Git 2.51, the lock files for both would be created, but
the 'commit' phase would abruptly end leaving the lock files.

The second commit fixes this by ensuring that on case-insensitive
filesystems we lowercase the refnames for availability check to ensure
F/D are caught and reported to the user.

4. When fetching references with D/F conflict:

  - refs/heads/Foo/bar
  - refs/heads/foo

The creation of the second reference's lock in `lock_raw_ref()` catches
the D/F conflict, but we mark this as a GENERIC error. By categorizing
this as non-GENERIC, we can allow the updates to continue while
rejecting this specific error.

This also applies to D/F conflicts in case-sensitive systems where there
exists a lock in the repo 'refs/heads/foo/bar.lock' causing a conflict
while fetching 'refs/heads/foo'.

- Karthik

[1]: https://lore.kernel.org/all/YQXPR01MB3046197EF39296549EE6DD669A33A@YQXPR01MB3046.CANPRD01.PROD.OUTLOOK.COM/

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
Changes in v4:
- Fixes to typos in commit messages.
- Use curly braces for if..else clause with inlined comments.
- Link to v3: https://lore.kernel.org/r/20250913-587-git-fetch-1-fails-fetches-on-case-insensitive-repositories-v3-0-195569740b57@gmail.com

Changes in v3:
- Rename duplicate_reference_case_cmp() to
  transaction_has_case_conflicting_update() and add comments.
- Improve commit messages.
- Add an additional test in the 4th commit to showcase D/F conflicts in
  case-sensistive file systems.
- Link to v2: https://lore.kernel.org/r/20250908-587-git-fetch-1-fails-fetches-on-case-insensitive-repositories-v2-0-b2eb2459befb@gmail.com

Changes in v2:
- This version fixes two more issues:
  - Fetching while locks already exist in the repository
  - D/F conflicts while fetching
- Add a specific error to the first case, so we can nicely show a
  relevant error. Also check explicitly that the issue is due to
  case-insensitive filesystems.
- Cleanup the commit messages.
- Use `string_list_append_nodup()` with `strbuf_detach`, reducing the
  number of allocations.

---
 builtin/fetch.c       |  21 ++++++++--
 refs.c                |  11 ++++-
 refs.h                |   2 +
 refs/files-backend.c  |  78 ++++++++++++++++++++++++++++------
 t/t1400-update-ref.sh |  53 +++++++++++++++++++++++
 t/t5510-fetch.sh      | 114 +++++++++++++++++++++++++++++++++++++++++++++++++-
 6 files changed, 262 insertions(+), 17 deletions(-)

Karthik Nayak (4):
      refs/files: catch conflicts on case-insensitive file-systems
      refs/files: use correct error type when lock exists
      refs/files: handle F/D conflicts in case-insensitive FS
      refs/files: handle D/F conflicts during locking

Range-diff versus v3:

1:  ec89ebee70 ! 1:  203a32f814 refs/files: catch conflicts on case-insensitive file-systems
    @@ Metadata
      ## Commit message ##
         refs/files: catch conflicts on case-insensitive file-systems
     
    -    During the 'prepare' phase of reference transaction in the files
    +    During the 'prepare' phase of a reference transaction in the files
         backend, we create the lock files for references to be created. When
         using batched updates on case-insensitive filesystems, the entire
         batched updates would be aborted if there are conflicting names such as:
    @@ Commit message
         This should be an okay compromise since with the files backend, there is
         no scenario possible where we would retain all colliding references.
     
    -    Let's also be more pro-active and notify users on case-insensitive
    +    Let's also be more proactive and notify users on case-insensitive
         filesystems about such problems by providing a brief about the issue
         while also recommending using the reftable backend, which doesn't have
         the same issue.
2:  e231d44125 ! 2:  f06c1a89b9 refs/files: use correct error type when lock exists
    @@ Commit message
         reference exists, then `lock_raw_ref()` throws:
     
             - REF_TRANSACTION_ERROR_CASE_CONFLICT: when there is a conflict
    -        because transaction contains conflicting references while being on a
    -        case-insensitive filesystem.
    +        because the transaction contains conflicting references while being
    +        on a case-insensitive filesystem.
     
             - REF_TRANSACTION_ERROR_GENERIC: for all other errors.
     
    @@ refs/files-backend.c: static enum ref_transaction_error lock_raw_ref(struct file
     -				ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
     +			if (myerr == EEXIST) {
     +				if (ignore_case &&
    -+				    transaction_has_case_conflicting_update(transaction, update))
    ++				    transaction_has_case_conflicting_update(transaction, update)) {
     +					/*
     +					 * In case-insensitive filesystems, ensure that conflicts within a
     +					 * given transaction are handled. Pre-existing refs on a
     +					 * case-insensitive system will be overridden without any issue.
     +					 */
     +					ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
    -+				else
    ++				} else {
     +					/*
     +					 * Pre-existing case-conflicting reference locks should also be
     +					 * specially categorized to avoid failing all batched updates.
     +					 */
     +					ret = REF_TRANSACTION_ERROR_CREATE_EXISTS;
    ++				}
     +			}
     +
      			goto error_return;
3:  0100f5a48d ! 3:  e1b3f8106c refs/files: handle F/D conflicts in case-insensitive FS
    @@ Commit message
         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
    +    To do this, simply use a `struct strbuf` to convert the refname to
    +    lowercase 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.
     
4:  64def2c0fb = 4:  5c4886a1fb refs/files: handle D/F conflicts during locking


base-commit: c44beea485f0f2feaf460e2ac87fdd5608d63cf0
change-id: 20250822-587-git-fetch-1-fails-fetches-on-case-insensitive-repositories-0a187c7ac41e

Thanks
- Karthik

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

[Karthik Nayak]

During the 'prepare' phase of a reference transaction in the files
backend, we create the lock files for references to be created. When
using batched updates on case-insensitive filesystems, the entire
batched updates would be aborted if there are conflicting names such as:

  refs/heads/Foo
  refs/heads/foo

This affects all commands which were migrated to use batched updates in
Git 2.51, including 'git-fetch(1)' and 'git-receive-pack(1)'. Before
that, reference updates would be applied serially with one transaction
used per update. When users fetched multiple references on
case-insensitive systems, subsequent references would simply overwrite
any earlier references. So when fetching:

  refs/heads/foo: 5f34ec0bfeac225b1c854340257a65b106f70ea6
  refs/heads/Foo: ec3053b0977e83d9b67fc32c4527a117953994f3
  refs/heads/sample: 2eefd1150e06d8fca1ddfa684dec016f36bf4e56

The user would simply end up with:

  refs/heads/foo: ec3053b0977e83d9b67fc32c4527a117953994f3
  refs/heads/sample: 2eefd1150e06d8fca1ddfa684dec016f36bf4e56

This is buggy behavior since the user is never informed about the
overrides performed and missing references. Nevertheless, the user is
left with a working repository with a subset of the references. Since
Git 2.51, in such situations fetches would simply fail without updating
any references. Which is also buggy behavior and worse off since the
user is left without any references.

The error is triggered in `lock_raw_ref()` where the files backend
attempts to create a lock file. When a lock file already exists the
function returns a 'REF_TRANSACTION_ERROR_GENERIC'. When this happens,
the entire batched updates, not individual operation, is aborted as if
it were in a transaction.

Change this to return 'REF_TRANSACTION_ERROR_CASE_CONFLICT' instead to
aid the batched update mechanism to simply reject such errors. The
change only affects batched updates since batched updates will reject
individual updates with non-generic errors. So specifically this would
only affect:

    1. git fetch
    2. git receive-pack
    3. git update-ref --batch-updates

This bubbles the error type up to `files_transaction_prepare()` which
tries to lock each reference update. So if the locking fails, we check
if the rejection type can be ignored, which is done by calling
`ref_transaction_maybe_set_rejected()`.

As the error type is now 'REF_TRANSACTION_ERROR_CASE_CONFLICT',
the specific reference update would simply be rejected, while other
updates in the transaction would continue to be applied. This allows
partial application of references in case-insensitive filesystems when
fetching colliding references.

While the earlier implementation allowed the last reference to be
applied overriding the initial references, this change would allow the
first reference to be applied while rejecting consequent collisions.
This should be an okay compromise since with the files backend, there is
no scenario possible where we would retain all colliding references.

Let's also be more proactive and notify users on case-insensitive
filesystems about such problems by providing a brief about the issue
while also recommending using the reftable backend, which doesn't have
the same issue.

Reported-by: Joe Drew <joe.drew@indexexchange.com>
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/fetch.c       | 21 +++++++++++++++++---
 refs.c                |  2 ++
 refs.h                |  2 ++
 refs/files-backend.c  | 33 +++++++++++++++++++++++++++-----
 t/t1400-update-ref.sh | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
 t/t5510-fetch.sh      | 22 ++++++++++++++++++++-
 6 files changed, 124 insertions(+), 9 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 24645c4653..c7ff3480fb 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1643,7 +1643,8 @@ static int set_head(const struct ref *remote_refs, struct remote *remote)
 
 struct ref_rejection_data {
 	int *retcode;
-	int conflict_msg_shown;
+	bool conflict_msg_shown;
+	bool case_sensitive_msg_shown;
 	const char *remote_name;
 };
 
@@ -1657,11 +1658,25 @@ static void ref_transaction_rejection_handler(const char *refname,
 {
 	struct ref_rejection_data *data = cb_data;
 
-	if (err == REF_TRANSACTION_ERROR_NAME_CONFLICT && !data->conflict_msg_shown) {
+	if (err == REF_TRANSACTION_ERROR_CASE_CONFLICT && ignore_case &&
+	    !data->case_sensitive_msg_shown) {
+		error(_("You're on a case-insensitive filesystem, and the remote you are\n"
+			"trying to fetch from has references that only differ in casing. It\n"
+			"is impossible to store such references with the 'files' backend. You\n"
+			"can either accept this as-is, in which case you won't be able to\n"
+			"store all remote references on disk. Or you can alternatively\n"
+			"migrate your repository to use the 'reftable' backend with the\n"
+			"following command:\n\n    git refs migrate --ref-format=reftable\n\n"
+			"Please keep in mind that not all implementations of Git support this\n"
+			"new format yet. So if you use tools other than Git to access this\n"
+			"repository it may not be an option to migrate to reftables.\n"));
+		data->case_sensitive_msg_shown = true;
+	} else if (err == REF_TRANSACTION_ERROR_NAME_CONFLICT &&
+		   !data->conflict_msg_shown) {
 		error(_("some local refs could not be updated; try running\n"
 			" 'git remote prune %s' to remove any old, conflicting "
 			"branches"), data->remote_name);
-		data->conflict_msg_shown = 1;
+		data->conflict_msg_shown = true;
 	} else {
 		const char *reason = ref_transaction_error_msg(err);
 
diff --git a/refs.c b/refs.c
index bfdbe718b7..4c1c339ed9 100644
--- a/refs.c
+++ b/refs.c
@@ -3321,6 +3321,8 @@ const char *ref_transaction_error_msg(enum ref_transaction_error err)
 		return "invalid new value provided";
 	case REF_TRANSACTION_ERROR_EXPECTED_SYMREF:
 		return "expected symref but found regular ref";
+	case REF_TRANSACTION_ERROR_CASE_CONFLICT:
+		return "reference conflict due to case-insensitive filesystem";
 	default:
 		return "unknown failure";
 	}
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,
 };
 
 /*
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 088b52c740..01df32904b 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -647,6 +647,26 @@ static void unlock_ref(struct ref_lock *lock)
 	}
 }
 
+/*
+ * Check if the transaction has another update with a case-insensitive refname
+ * match.
+ *
+ * If the update is part of the transaction, we only check up to that index.
+ * Further updates are expected to call this function to match previous indices.
+ */
+static bool transaction_has_case_conflicting_update(struct ref_transaction *transaction,
+						    struct ref_update *update)
+{
+	for (size_t i = 0; i < transaction->nr; i++) {
+		if (transaction->updates[i] == update)
+			break;
+
+		if (!strcasecmp(transaction->updates[i]->refname, update->refname))
+			return true;
+	}
+	return false;
+}
+
 /*
  * Lock refname, without following symrefs, and set *lock_p to point
  * at a newly-allocated lock object. Fill in lock->old_oid, referent,
@@ -677,16 +697,17 @@ static void unlock_ref(struct ref_lock *lock)
  * - Generate informative error messages in the case of failure
  */
 static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs,
-					       struct ref_update *update,
+					       struct ref_transaction *transaction,
 					       size_t update_idx,
 					       int mustexist,
 					       struct string_list *refnames_to_check,
-					       const struct string_list *extras,
 					       struct ref_lock **lock_p,
 					       struct strbuf *referent,
 					       struct strbuf *err)
 {
 	enum ref_transaction_error ret = REF_TRANSACTION_ERROR_GENERIC;
+	struct ref_update *update = transaction->updates[update_idx];
+	const struct string_list *extras = &transaction->refnames;
 	const char *refname = update->refname;
 	unsigned int *type = &update->type;
 	struct ref_lock *lock;
@@ -776,6 +797,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 &&
+			    transaction_has_case_conflicting_update(transaction, update))
+				ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
 			goto error_return;
 		}
 	}
@@ -2583,9 +2607,8 @@ static enum ref_transaction_error lock_ref_for_update(struct files_ref_store *re
 	if (lock) {
 		lock->count++;
 	} else {
-		ret = lock_raw_ref(refs, update, update_idx, mustexist,
-				   refnames_to_check, &transaction->refnames,
-				   &lock, &referent, err);
+		ret = lock_raw_ref(refs, transaction, update_idx, mustexist,
+				   refnames_to_check, &lock, &referent, err);
 		if (ret) {
 			char *reason;
 
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 96648a6e5d..08d5df2af7 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -2294,6 +2294,59 @@ do
 		)
 	'
 
+	test_expect_success CASE_INSENSITIVE_FS,REFFILES "stdin $type batch-updates existing reference" '
+		git init repo &&
+		test_when_finished "rm -fr repo" &&
+		(
+			cd repo &&
+			test_commit one &&
+			old_head=$(git rev-parse HEAD) &&
+			test_commit two &&
+			head=$(git rev-parse HEAD) &&
+
+			{
+				format_command $type "create refs/heads/foo" "$head" &&
+				format_command $type "create refs/heads/ref" "$old_head" &&
+				format_command $type "create refs/heads/Foo" "$old_head"
+			} >stdin &&
+			git update-ref $type --stdin --batch-updates <stdin >stdout &&
+
+			echo $head >expect &&
+			git rev-parse refs/heads/foo >actual &&
+			echo $old_head >expect &&
+			git rev-parse refs/heads/ref >actual &&
+			test_cmp expect actual &&
+			test_grep -q "reference conflict due to case-insensitive filesystem" stdout
+		)
+	'
+
+	test_expect_success CASE_INSENSITIVE_FS "stdin $type batch-updates existing reference" '
+		git init --ref-format=reftable repo &&
+		test_when_finished "rm -fr repo" &&
+		(
+			cd repo &&
+			test_commit one &&
+			old_head=$(git rev-parse HEAD) &&
+			test_commit two &&
+			head=$(git rev-parse HEAD) &&
+
+			{
+				format_command $type "create refs/heads/foo" "$head" &&
+				format_command $type "create refs/heads/ref" "$old_head" &&
+				format_command $type "create refs/heads/Foo" "$old_head"
+			} >stdin &&
+			git update-ref $type --stdin --batch-updates <stdin >stdout &&
+
+			echo $head >expect &&
+			git rev-parse refs/heads/foo >actual &&
+			echo $old_head >expect &&
+			git rev-parse refs/heads/ref >actual &&
+			test_cmp expect actual &&
+			git rev-parse refs/heads/Foo >actual &&
+			test_cmp expect actual
+		)
+	'
+
 	test_expect_success "stdin $type batch-updates delete incorrect symbolic ref" '
 		git init repo &&
 		test_when_finished "rm -fr repo" &&
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index ebc696546b..57f60da81b 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -47,7 +47,13 @@ test_expect_success "clone and setup child repos" '
 		git config set branch.main.merge refs/heads/one
 	) &&
 	git clone . bundle &&
-	git clone . seven
+	git clone . seven &&
+	git clone --ref-format=reftable . case_sensitive &&
+	(
+		cd case_sensitive &&
+		git branch branch1 &&
+		git branch bRanch1
+	)
 '
 
 test_expect_success "fetch test" '
@@ -1526,6 +1532,20 @@ test_expect_success SYMLINKS 'clone does not get confused by a D/F conflict' '
 	test_path_is_missing whoops
 '
 
+test_expect_success CASE_INSENSITIVE_FS,REFFILES 'existing references in a case insensitive filesystem' '
+	test_when_finished rm -rf case_insensitive &&
+	(
+		git init --bare case_insensitive &&
+		cd case_insensitive &&
+		git remote add origin -- ../case_sensitive &&
+		test_must_fail git fetch -f origin "refs/heads/*:refs/heads/*" 2>err &&
+		test_grep "You${SQ}re on a case-insensitive filesystem" err &&
+		git rev-parse refs/heads/main >expect &&
+		git rev-parse refs/heads/branch1 >actual &&
+		test_cmp expect actual
+	)
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 

-- 
2.51.0

[PATCH v4 2/4] refs/files: use correct error type when lock exists

[Karthik Nayak]

When fetching references into a repository, if a lock for a particular
reference exists, then `lock_raw_ref()` throws:

    - REF_TRANSACTION_ERROR_CASE_CONFLICT: when there is a conflict
    because the transaction contains conflicting references while being
    on a case-insensitive filesystem.

    - REF_TRANSACTION_ERROR_GENERIC: for all other errors.

The latter causes the entire set of batched updates to fail, even in
case sensitive filessystems.

Instead, return a 'REF_TRANSACTION_ERROR_CREATE_EXISTS' error. This
allows batched updates to reject the individual update which conflicts
with the existing file, while updating the rest of the references.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs/files-backend.c | 21 ++++++++++++++++++---
 t/t5510-fetch.sh     | 26 ++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 01df32904b..d1af5d6bc7 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -797,9 +797,24 @@ 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 &&
-			    transaction_has_case_conflicting_update(transaction, update))
-				ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
+			if (myerr == EEXIST) {
+				if (ignore_case &&
+				    transaction_has_case_conflicting_update(transaction, update)) {
+					/*
+					 * In case-insensitive filesystems, ensure that conflicts within a
+					 * given transaction are handled. Pre-existing refs on a
+					 * case-insensitive system will be overridden without any issue.
+					 */
+					ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
+				} else {
+					/*
+					 * Pre-existing case-conflicting reference locks should also be
+					 * specially categorized to avoid failing all batched updates.
+					 */
+					ret = REF_TRANSACTION_ERROR_CREATE_EXISTS;
+				}
+			}
+
 			goto error_return;
 		}
 	}
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 57f60da81b..6f8db0ace4 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -1546,6 +1546,32 @@ test_expect_success CASE_INSENSITIVE_FS,REFFILES 'existing references in a case
 	)
 '
 
+test_expect_success REFFILES 'existing reference lock in repo' '
+	test_when_finished rm -rf base repo &&
+	(
+		git init --ref-format=reftable base &&
+		cd base &&
+		echo >file update &&
+		git add . &&
+		git commit -m "updated" &&
+		git branch -M main &&
+
+		git update-ref refs/heads/foo @ &&
+		git update-ref refs/heads/branch @ &&
+		cd .. &&
+
+		git init --ref-format=files --bare repo &&
+		cd repo &&
+		git remote add origin ../base &&
+		touch refs/heads/foo.lock &&
+		test_must_fail git fetch -f origin "refs/heads/*:refs/heads/*" 2>err &&
+		test_grep "error: fetching ref refs/heads/foo failed: reference already exists" err &&
+		git rev-parse refs/heads/main >expect &&
+		git rev-parse refs/heads/branch >actual &&
+		test_cmp expect actual
+	)
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 

-- 
2.51.0

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

[Karthik Nayak]

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
lowercase 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.

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 d1af5d6bc7..bfdf85121a 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -906,8 +906,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));
+		} else {
+			item = string_list_append(refnames_to_check, refname);
+		}
+
 		item->util = xmalloc(sizeof(update_idx));
 		memcpy(item->util, &update_idx, sizeof(update_idx));
 	}
@@ -2832,7 +2847,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 6f8db0ace4..08dbea6503 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
 	)
 '
 
@@ -1572,6 +1578,20 @@ test_expect_success REFFILES 'existing reference lock in repo' '
 	)
 '
 
+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
 

-- 
2.51.0

[PATCH v4 4/4] refs/files: handle D/F conflicts during locking

[Karthik Nayak]

The previous commit added the necessary validation and checks for F/D
conflicts in the files backend when working on case insensitive systems.

There is still a possibility for D/F conflicts. This is a different from
the F/D since for F/D conflicts, there would not be a conflict during
the lock creation phase:

    refs/heads/foo.lock
    refs/heads/foo/bar.lock

However there would be a conflict when the locks are committed, since we
cannot have 'refs/heads/foo/bar' and 'refs/heads/foo'. These kinds of
conflicts are checked and resolved in
`refs_verify_refnames_available()`, so the previous commit ensured that
for case-insensitive filesystems, we would lowercase the inputs to that
function.

For D/F conflicts, there is a conflict during the lock creation phase
itself:

    refs/heads/foo/bar.lock
    refs/heads/foo.lock

As in `lock_raw_ref()` after creating the lock, we also check for D/F
conflicts. This can occur in case-insensitive filesystems when trying to
fetch case-conflicted references like:

    refs/heads/Foo/new
    refs/heads/foo

D/F conflicts can also occur in case-sensitive filesystems, when the
repository already contains a directory with a lock file
'refs/heads/foo/bar.lock' and trying to fetch 'refs/heads/foo'. This
doesn't concern directories containing garbage files as those are
handled on a higher level.

To fix this, simply categorize the error as a name conflict. Also remove
this reference from the list of valid refnames for availability checks.
By categorizing the error and removing it from the list of valid
references, batched updates now knows to reject such reference updates
and apply the other reference updates.

Fix a small typo in `ref_transaction_maybe_set_rejected()` while here.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs.c               |  9 ++++++++-
 refs/files-backend.c | 11 ++++++-----
 t/t5510-fetch.sh     | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 60 insertions(+), 6 deletions(-)

diff --git a/refs.c b/refs.c
index 4c1c339ed9..e7109ea5fe 100644
--- a/refs.c
+++ b/refs.c
@@ -1223,7 +1223,7 @@ int ref_transaction_maybe_set_rejected(struct ref_transaction *transaction,
 		return 0;
 
 	if (!transaction->rejections)
-		BUG("transaction not inititalized with failure support");
+		BUG("transaction not initialized with failure support");
 
 	/*
 	 * Don't accept generic errors, since these errors are not user
@@ -1232,6 +1232,13 @@ int ref_transaction_maybe_set_rejected(struct ref_transaction *transaction,
 	if (err == REF_TRANSACTION_ERROR_GENERIC)
 		return 0;
 
+	/*
+	 * Rejected refnames shouldn't be considered in the availability
+	 * checks, so remove them from the list.
+	 */
+	string_list_remove(&transaction->refnames,
+			   transaction->updates[update_idx]->refname, 0);
+
 	transaction->updates[update_idx]->rejection_err = err;
 	ALLOC_GROW(transaction->rejections->update_indices,
 		   transaction->rejections->nr + 1,
diff --git a/refs/files-backend.c b/refs/files-backend.c
index bfdf85121a..4a0343d827 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -870,6 +870,7 @@ static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs,
 				goto error_return;
 			} else if (remove_dir_recursively(&ref_file,
 							  REMOVE_DIR_EMPTY_ONLY)) {
+				ret = REF_TRANSACTION_ERROR_NAME_CONFLICT;
 				if (refs_verify_refname_available(
 						    &refs->base, refname,
 						    extras, NULL, 0, err)) {
@@ -877,14 +878,14 @@ static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs,
 					 * The error message set by
 					 * verify_refname_available() is OK.
 					 */
-					ret = REF_TRANSACTION_ERROR_NAME_CONFLICT;
 					goto error_return;
 				} else {
 					/*
-					 * We can't delete the directory,
-					 * but we also don't know of any
-					 * references that it should
-					 * contain.
+					 * Directory conflicts can occur if there
+					 * is an existing lock file in the directory
+					 * or if the filesystem is case-insensitive
+					 * and the directory contains a valid reference
+					 * but conflicts with the update.
 					 */
 					strbuf_addf(err, "there is a non-empty directory '%s' "
 						    "blocking reference '%s'",
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 08dbea6503..6b2739db26 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -59,6 +59,12 @@ test_expect_success "clone and setup child repos" '
 		cd case_sensitive_fd &&
 		git branch foo/bar &&
 		git branch Foo
+	) &&
+	git clone --ref-format=reftable . case_sensitive_df &&
+	(
+		cd case_sensitive_df &&
+		git branch Foo/bar &&
+		git branch foo
 	)
 '
 
@@ -1592,6 +1598,46 @@ test_expect_success CASE_INSENSITIVE_FS,REFFILES 'F/D conflict on case insensiti
 	)
 '
 
+test_expect_success CASE_INSENSITIVE_FS,REFFILES 'D/F 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_df &&
+		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_expect_success REFFILES 'D/F conflict on case sensitive filesystem with lock' '
+	(
+		git init --ref-format=reftable base &&
+		cd base &&
+		echo >file update &&
+		git add . &&
+		git commit -m "updated" &&
+		git branch -M main &&
+
+		git update-ref refs/heads/foo @ &&
+		git update-ref refs/heads/branch @ &&
+		cd .. &&
+
+		git init --ref-format=files --bare repo &&
+		cd repo &&
+		git remote add origin ../base &&
+		mkdir refs/heads/foo &&
+		touch refs/heads/foo/random.lock &&
+		test_must_fail git fetch origin "refs/heads/*:refs/heads/*" 2>err &&
+		test_grep "some local refs could not be updated; try running" err &&
+		git rev-parse refs/heads/main >expect &&
+		git rev-parse refs/heads/branch >actual &&
+		test_cmp expect actual
+	)
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 

-- 
2.51.0

Re: [PATCH v4 0/4] refs/files: fix issues with git-fetch on case-insensitive FS

[Justin Tobler]

On 25/09/17 05:25PM, Karthik Nayak wrote:
> Changes in v4:
> - Fixes to typos in commit messages.
> - Use curly braces for if..else clause with inlined comments.
> - Link to v3: https://lore.kernel.org/r/20250913-587-git-fetch-1-fails-fetches-on-case-insensitive-repositories-v3-0-195569740b57@gmail.com

Looking at the range-diff, this version looks good to me.

Thanks,
-Justin

Re: [PATCH v4 0/4] refs/files: fix issues with git-fetch on case-insensitive FS

[Toon Claes]

Justin Tobler <jltobler@gmail.com> writes:

> Looking at the range-diff, this version looks good to me.
>
> Thanks,
> -Justin
>

I also have got nothing to add.

-- 
Cheers,
Toon

Re: [PATCH v4 0/4] refs/files: fix issues with git-fetch on case-insensitive FS

[Junio C Hamano]

Toon Claes <toon@iotcl.com> writes:

> Justin Tobler <jltobler@gmail.com> writes:
>
>> Looking at the range-diff, this version looks good to me.
>>
>> Thanks,
>> -Justin
>>
>
> I also have got nothing to add.

Thanks, all.  Let's declare victory and mark it for 'next' then.

Re: [PATCH v4 0/4] refs/files: fix issues with git-fetch on case-insensitive FS

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> Changes in v4:
> - Fixes to typos in commit messages.
> - Use curly braces for if..else clause with inlined comments.
> - Link to v3: https://lore.kernel.org/r/20250913-587-git-fetch-1-fails-fetches-on-case-insensitive-repositories-v3-0-195569740b57@gmail.com

Thanks for a pleasant read.  Will replace.