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

[Patrick Steinhardt <ps@pks.im>]

On Mon, Sep 08, 2025 at 02:37:35PM +0200, Karthik Nayak wrote:
> 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.

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

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