git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] refs/files: fix issues with git-fetch on case-insensitive FS
@ 2025-09-02  8:34 Karthik Nayak
  2025-09-02  8:34 ` [PATCH 1/2] refs/files: use correct error type when locking fails Karthik Nayak
  2025-09-02  8:34 ` [PATCH 2/2] refs/files: handle F/D conflicts in case-insensitive FS Karthik Nayak
  0 siblings, 2 replies; 12+ messages in thread
From: Karthik Nayak @ 2025-09-02  8:34 UTC (permalink / raw)
  To: git; +Cc: Karthik Nayak, joe.drew, peff, ps, gitster

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/2] refs/files: use correct error type when locking fails
  2025-09-02  8:34 [PATCH 0/2] refs/files: fix issues with git-fetch on case-insensitive FS Karthik Nayak
@ 2025-09-02  8:34 ` Karthik Nayak
  2025-09-02 21:22   ` Chris Torek
                     ` (2 more replies)
  2025-09-02  8:34 ` [PATCH 2/2] refs/files: handle F/D conflicts in case-insensitive FS Karthik Nayak
  1 sibling, 3 replies; 12+ messages in thread
From: Karthik Nayak @ 2025-09-02  8:34 UTC (permalink / raw)
  To: git; +Cc: Karthik Nayak, joe.drew, peff, ps, gitster

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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/2] refs/files: handle F/D conflicts in case-insensitive FS
  2025-09-02  8:34 [PATCH 0/2] refs/files: fix issues with git-fetch on case-insensitive FS Karthik Nayak
  2025-09-02  8:34 ` [PATCH 1/2] refs/files: use correct error type when locking fails Karthik Nayak
@ 2025-09-02  8:34 ` Karthik Nayak
  2025-09-03  7:40   ` Patrick Steinhardt
  2025-09-03 18:16   ` Junio C Hamano
  1 sibling, 2 replies; 12+ messages in thread
From: Karthik Nayak @ 2025-09-02  8:34 UTC (permalink / raw)
  To: git; +Cc: Karthik Nayak, joe.drew, peff, ps, gitster

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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] refs/files: use correct error type when locking fails
  2025-09-02  8:34 ` [PATCH 1/2] refs/files: use correct error type when locking fails Karthik Nayak
@ 2025-09-02 21:22   ` Chris Torek
  2025-09-04 20:24     ` Karthik Nayak
  2025-09-03  7:40   ` Patrick Steinhardt
  2025-09-03 16:53   ` Junio C Hamano
  2 siblings, 1 reply; 12+ messages in thread
From: Chris Torek @ 2025-09-02 21:22 UTC (permalink / raw)
  To: Karthik Nayak; +Cc: git, joe.drew, peff, ps, gitster

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] refs/files: use correct error type when locking fails
  2025-09-02  8:34 ` [PATCH 1/2] refs/files: use correct error type when locking fails Karthik Nayak
  2025-09-02 21:22   ` Chris Torek
@ 2025-09-03  7:40   ` Patrick Steinhardt
  2025-09-03 10:38     ` Karthik Nayak
  2025-09-03 16:53   ` Junio C Hamano
  2 siblings, 1 reply; 12+ messages in thread
From: Patrick Steinhardt @ 2025-09-03  7:40 UTC (permalink / raw)
  To: Karthik Nayak; +Cc: git, joe.drew, peff, gitster

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] refs/files: handle F/D conflicts in case-insensitive FS
  2025-09-02  8:34 ` [PATCH 2/2] refs/files: handle F/D conflicts in case-insensitive FS Karthik Nayak
@ 2025-09-03  7:40   ` Patrick Steinhardt
  2025-09-03 18:16   ` Junio C Hamano
  1 sibling, 0 replies; 12+ messages in thread
From: Patrick Steinhardt @ 2025-09-03  7:40 UTC (permalink / raw)
  To: Karthik Nayak; +Cc: git, joe.drew, peff, gitster

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] refs/files: use correct error type when locking fails
  2025-09-03  7:40   ` Patrick Steinhardt
@ 2025-09-03 10:38     ` Karthik Nayak
  2025-09-03 11:48       ` Patrick Steinhardt
  0 siblings, 1 reply; 12+ messages in thread
From: Karthik Nayak @ 2025-09-03 10:38 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: git, joe.drew, peff, gitster

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] refs/files: use correct error type when locking fails
  2025-09-03 10:38     ` Karthik Nayak
@ 2025-09-03 11:48       ` Patrick Steinhardt
  0 siblings, 0 replies; 12+ messages in thread
From: Patrick Steinhardt @ 2025-09-03 11:48 UTC (permalink / raw)
  To: Karthik Nayak; +Cc: git, joe.drew, peff, gitster

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] refs/files: use correct error type when locking fails
  2025-09-02  8:34 ` [PATCH 1/2] refs/files: use correct error type when locking fails Karthik Nayak
  2025-09-02 21:22   ` Chris Torek
  2025-09-03  7:40   ` Patrick Steinhardt
@ 2025-09-03 16:53   ` Junio C Hamano
  2 siblings, 0 replies; 12+ messages in thread
From: Junio C Hamano @ 2025-09-03 16:53 UTC (permalink / raw)
  To: Karthik Nayak; +Cc: git, joe.drew, peff, ps

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] refs/files: handle F/D conflicts in case-insensitive FS
  2025-09-02  8:34 ` [PATCH 2/2] refs/files: handle F/D conflicts in case-insensitive FS Karthik Nayak
  2025-09-03  7:40   ` Patrick Steinhardt
@ 2025-09-03 18:16   ` Junio C Hamano
  2025-09-04 22:43     ` Junio C Hamano
  1 sibling, 1 reply; 12+ messages in thread
From: Junio C Hamano @ 2025-09-03 18:16 UTC (permalink / raw)
  To: Karthik Nayak; +Cc: git, joe.drew, peff, ps

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] refs/files: use correct error type when locking fails
  2025-09-02 21:22   ` Chris Torek
@ 2025-09-04 20:24     ` Karthik Nayak
  0 siblings, 0 replies; 12+ messages in thread
From: Karthik Nayak @ 2025-09-04 20:24 UTC (permalink / raw)
  To: Chris Torek; +Cc: git, joe.drew, peff, ps, gitster

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] refs/files: handle F/D conflicts in case-insensitive FS
  2025-09-03 18:16   ` Junio C Hamano
@ 2025-09-04 22:43     ` Junio C Hamano
  0 siblings, 0 replies; 12+ messages in thread
From: Junio C Hamano @ 2025-09-04 22:43 UTC (permalink / raw)
  To: Karthik Nayak; +Cc: git, joe.drew, peff, ps

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



^ permalink raw reply related	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2025-09-04 22:43 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-09-02  8:34 [PATCH 0/2] refs/files: fix issues with git-fetch on case-insensitive FS Karthik Nayak
2025-09-02  8:34 ` [PATCH 1/2] refs/files: use correct error type when locking fails Karthik Nayak
2025-09-02 21:22   ` Chris Torek
2025-09-04 20:24     ` Karthik Nayak
2025-09-03  7:40   ` Patrick Steinhardt
2025-09-03 10:38     ` Karthik Nayak
2025-09-03 11:48       ` Patrick Steinhardt
2025-09-03 16:53   ` Junio C Hamano
2025-09-02  8:34 ` [PATCH 2/2] refs/files: handle F/D conflicts in case-insensitive FS Karthik Nayak
2025-09-03  7:40   ` Patrick Steinhardt
2025-09-03 18:16   ` Junio C Hamano
2025-09-04 22:43     ` Junio C Hamano

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).