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

[Patrick Steinhardt <ps@pks.im>]

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/

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

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