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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 3, 2025, 16:53 UTC
Message-ID: <xmqqv7lzfpeu.fsf@gitster.g>
In-Reply-To: <20250902-587-git-fetch-1-fails-fetches-on-case-insensitive-repositories-v1-1-35e69bbb507d@gmail.com>

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

Show 25 quoted lines

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

Show 5 quoted lines

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

Show 5 quoted lines

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

Show 11 quoted lines

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