Re: [PATCH v3 1/4] refs/files: catch conflicts on case-insensitive file-systems

> backend, we create the lock files for references to be created. When
> using batched updates on case-insensitive filesystems, the entire
> batched updates 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, reference 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 informed 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 updating
> any references. Which is also buggy behavior and worse off since the
> user is left without any references.
> 
> 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'. When this happens,
> the entire batched updates, not individual operation, is aborted as if
> it were in a transaction.
> 
> Change this to return 'REF_TRANSACTION_ERROR_CASE_CONFLICT' instead to
> aid the batched update mechanism to simply reject such errors. 

> 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

> 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_CASE_CONFLICT',
> 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.
> 
> 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.
> 
> 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.
> 
> Reported-by: Joe Drew <joe.drew@indexexchange.com>
> Helped-by: Patrick Steinhardt <ps@pks.im>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  builtin/fetch.c       | 21 +++++++++++++++++---
>  refs.c                |  2 ++
>  refs.h                |  2 ++
>  refs/files-backend.c  | 33 +++++++++++++++++++++++++++-----
>  t/t1400-update-ref.sh | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  t/t5510-fetch.sh      | 22 ++++++++++++++++++++-
>  6 files changed, 124 insertions(+), 9 deletions(-)
> 
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 24645c4653..c7ff3480fb 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_CASE_CONFLICT && 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"));

> +/*
> + * Check if the transaction has another update with a case-insensitive refname
> + * match.
> + *
> + * If the update is part of the transaction, we only check up to that index.
> + * Further updates are expected to call this function to match previous indices.
> + */
> +static bool transaction_has_case_conflicting_update(struct ref_transaction *transaction,
> +						    struct ref_update *update)
> +{
> +	for (size_t i = 0; i < transaction->nr; i++) {
> +		if (transaction->updates[i] == update)
> +			break;
> +
> +		if (!strcasecmp(transaction->updates[i]->refname, update->refname))
> +			return true;
> +	}
> +	return false;
> +}

> +
>  /*
>   * Lock refname, without following symrefs, and set *lock_p to point
>   * at a newly-allocated lock object. Fill in lock->old_oid, referent,
> @@ -677,16 +697,17 @@ static void unlock_ref(struct ref_lock *lock)
>   * - Generate informative error messages in the case of failure
>   */
>  static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs,
> -					       struct ref_update *update,
> +					       struct ref_transaction *transaction,
>  					       size_t update_idx,
>  					       int mustexist,
>  					       struct string_list *refnames_to_check,
> -					       const struct string_list *extras,
>  					       struct ref_lock **lock_p,
>  					       struct strbuf *referent,
>  					       struct strbuf *err)
>  {
>  	enum ref_transaction_error ret = REF_TRANSACTION_ERROR_GENERIC;
> +	struct ref_update *update = transaction->updates[update_idx];
> +	const struct string_list *extras = &transaction->refnames;
>  	const char *refname = update->refname;
>  	unsigned int *type = &update->type;
>  	struct ref_lock *lock;
> @@ -776,6 +797,9 @@ 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 && ignore_case &&
> +			    transaction_has_case_conflicting_update(transaction, update))
> +				ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
>  			goto error_return;
>  		}
>  	}