Thread (43 messages) 43 messages, 5 authors, 2025-09-17

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

From: Karthik Nayak <hidden>
Date: 2025-09-11 09:35:54

Patrick Steinhardt [off-list ref] writes:
On Mon, Sep 08, 2025 at 02:37:35PM +0200, Karthik Nayak wrote:
quoted
diff --git a/refs.h b/refs.h
index eedbb599c5..41915086b3 100644
--- a/refs.h
+++ b/refs.h
@@ -31,6 +31,8 @@ enum ref_transaction_error {
 	REF_TRANSACTION_ERROR_INVALID_NEW_VALUE = -6,
 	/* Expected ref to be symref, but is a regular ref */
 	REF_TRANSACTION_ERROR_EXPECTED_SYMREF = -7,
+	/* Cannot create ref due to case-insensitive filesystem */
+	REF_TRANSACTION_ERROR_CASE_CONFLICT = -8,
Nice that we now have a specific error code for this error case. It
removes some of the guesswork we previously had to do.
Yeah Agreed. I dislike adding a common error which only affects a single
reference backend, but this is nicer.
quoted
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 088b52c740..58005d2732 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -647,6 +647,19 @@ static void unlock_ref(struct ref_lock *lock)
 	}
 }

+static bool duplicate_reference_case_cmp(struct ref_transaction *transaction,
+					 struct ref_update *update)
I think the name could use some improvement. How about
`transaction_has_case_conflicting_update()`?
That does read better, will change.
quoted
+{
+	for (size_t i = 0; i < transaction->nr; i++) {
+		if (transaction->updates[i] == update)
+			break;
Why do we break here? Shouldn't we continue?
We break because we only care about matching updates up to the index of
the provided updates. Further updates should recall the function.

I'll add a comment to explain this.
quoted
@@ -776,6 +790,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 &&
+			    duplicate_reference_case_cmp(transaction, update))
+				ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
 			goto error_return;
 		}
 	}
Okay. If we cannot lock the reference we now try to detect whether this
is because of a case conflict. That only catches the case though where
we have a case conflict in the same transaction, right? How about the
case where there's preexisting refs on disk that cause a conflict?
Existing references aren't an issue since in those situations we can
create the lock file. The issue here arises because within the same
transaction we try to create the lock file twice which causes the
conflict.
Patrick

Attachments

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help