Re: [PATCH v2 1/4] refs/files: catch conflicts on case-insensitive file-systems
From: Patrick Steinhardt <hidden>
Date: 2025-09-09 07:10:02
On Mon, Sep 08, 2025 at 02:37:35PM +0200, Karthik Nayak wrote:
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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()`?
+{
+ for (size_t i = 0; i < transaction->nr; i++) {
+ if (transaction->updates[i] == update)
+ break;Why do we break here? Shouldn't we continue?
quoted hunk ↗ jump to hunk
@@ -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? Patrick