Patrick Steinhardt <ps@xxxxxx> 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:
signature.asc
Description: PGP signature
