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

[Junio C Hamano <gitster@pobox.com>]

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.