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

[Patrick Steinhardt <ps@pks.im>]

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