Re: [PATCH v2 4/4] refs/files: handle D/F conflicts during locking

From: Patrick Steinhardt <ps@pks.im>
Date: Sep 9, 2025, 07:10 UTC
Message-ID: <aL_SznrF61hbUMBo@pks.im>
In-Reply-To: <20250908-587-git-fetch-1-fails-fetches-on-case-insensitive-repositories-v2-4-b2eb2459befb@gmail.com>

On Mon, Sep 08, 2025 at 02:37:38PM +0200, Karthik Nayak wrote:

> The previous commit, added the necessary validation and checks for F/D

s/commit,/commit/

Show 11 quoted lines

> diff --git a/refs.c b/refs.c
> index 4c1c339ed9..ec4f0e9502 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1232,6 +1232,12 @@ int ref_transaction_maybe_set_rejected(struct ref_transaction *transaction,
>  	if (err == REF_TRANSACTION_ERROR_GENERIC)
>  		return 0;
>  
> +	/*
> +	 * Remove this refname from the list of refnames used for validation
> +	 */

Nit: it's obvious that we remove the refname from that list, so the
comment is not helping much. It's much more important to explain _why_
we do that though to give readers the necessary context.

Show 26 quoted lines

> +	string_list_remove(&transaction->refnames,
> +			   transaction->updates[update_idx]->refname, 0);
> +
>  	transaction->updates[update_idx]->rejection_err = err;
>  	ALLOC_GROW(transaction->rejections->update_indices,
>  		   transaction->rejections->nr + 1,
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 85f2e14e93..ceeec272ff 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -852,6 +852,7 @@ static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs,
>  				goto error_return;
>  			} else if (remove_dir_recursively(&ref_file,
>  							  REMOVE_DIR_EMPTY_ONLY)) {
> +				ret = REF_TRANSACTION_ERROR_NAME_CONFLICT;
>  				if (refs_verify_refname_available(
>  						    &refs->base, refname,
>  						    extras, NULL, 0, err)) {
> @@ -859,7 +860,6 @@ static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs,
>  					 * The error message set by
>  					 * verify_refname_available() is OK.
>  					 */
> -					ret = REF_TRANSACTION_ERROR_NAME_CONFLICT;
>  					goto error_return;
>  				} else {
>  					/*

Hm, interesting. Previously we'd have returned a generic error in the `else` branch, which reads like this:

	} else {
		/*
		 * We can't delete the directory,
		 * but we also don't know of any
		 * references that it should
		 * contain.
		 */
		strbuf_addf(err, "there is a non-empty directory '%s' "
			    "blocking reference '%s'",
			    ref_file.buf, refname);
		goto error_return;
	}

So that directory contains something, even though we've previously verified that it shouldn't, if I understand correctly. The test case you add does seem to indicate that there are valid cases though where this can happen on a case insensitive filesystem?

If so, the comment definitely needs to be updated to explain this additional error case.

One more question is whether it's the correct thing to unconditionally return REF_TRANSACTION_ERROR_NAME_CONFLICT in that case now. Could it be that the directory exists but contains some garbage?

Patrick