From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 16 Sep 2025 07:51:20 GMT
Subject: Re: [PATCH v3 2/4] refs/files: use correct error type when lock exists
Message-ID: <aMkW-HOAsU8YZ8Lp@pks.im>
In-Reply-To: <20250913-587-git-fetch-1-fails-fetches-on-case-insensitive-repositories-v3-2-195569740b57@gmail.com>

On Sat, Sep 13, 2025 at 10:54:30PM +0200, Karthik Nayak wrote:
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 01df32904b..69e50a16db 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -797,9 +797,23 @@ 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 &&
> -			    transaction_has_case_conflicting_update(transaction, update))
> -				ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
> +			if (myerr == EEXIST) {
> +				if (ignore_case &&
> +				    transaction_has_case_conflicting_update(transaction, update))
> +					/*
> +					 * In case-insensitive filesystems, ensure that conflicts within a
> +					 * given transaction are handled. Pre-existing refs on a
> +					 * case-insensitive system will be overridden without any issue.
> +					 */
> +					ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
> +				else
> +					/*
> +					 * Pre-existing case-conflicting reference locks should also be
> +					 * specially categorized to avoid failing all batched updates.
> +					 */
> +					ret = REF_TRANSACTION_ERROR_CREATE_EXISTS;
> +			}

Tiniest nit: I think it would be preferable to use curly braces for such
multi-line comments. This nit isn't worth a reroll though.

Other than that my feedback got addressed, so this looks good to me.
Thanks!

Patrick