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

> 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