Patrick Steinhardt [off-list ref] writes:

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

quoted

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

s/commit,/commit/

Thanks, will change.

quoted

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.

Indeed, I'll add this instead

  Rejected refnames shouldn't be considered in the availability checks,
  so remove them from the list.

quoted

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

Yeah I think that comment needs to be rewritten.

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

This does affect case-sensitive systems too. I should've added a test
there as well to showcase this.

Basically if there is a lock file in a directory
'refs/heads/foo/bar.lock' while fetching 'refs/heads/foo', this would
reach this section of the code. Pre batched-updates, we would skip this
ref and update other refs. So it makes sense to retain this behavior.
I'll add in a test and amend the commit message to reflect this
behavior.

- Karthik