From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 09 Sep 2025 07:10:01 GMT
Subject: Re: [PATCH v2 2/4] refs/files: use correct error type when lock exists
Message-ID: <aL_SydrNjXdQrtFC@pks.im>
In-Reply-To: <20250908-587-git-fetch-1-fails-fetches-on-case-insensitive-repositories-v2-2-b2eb2459befb@gmail.com>

On Mon, Sep 08, 2025 at 02:37:36PM +0200, Karthik Nayak wrote:
> When fetching references into a repository, if a lock for a particular
> reference exists, then `lock_raw_ref()` throws the generic error
> 'REF_TRANSACTION_ERROR_GENERIC'. This causes the entire set of batched
> updates to fail.

This isn't quite accurate anymore as we may also raise
`REF_TRANSACTION_ERROR_CASE_CONFLICT` now.

> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 58005d2732..2730713d23 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -790,9 +790,13 @@ 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;
> +			if (myerr == EEXIST) {
> +				if (ignore_case && duplicate_reference_case_cmp(transaction, update))
> +					ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
> +				else
> +					ret = REF_TRANSACTION_ERROR_CREATE_EXISTS;
> +			}
> +
>  			goto error_return;
>  		}
>  	}

Hm. So if I understand correctly, we now return CREATE_EXISTS in case we
have a conflict with a preexisting case-conflicting reference, but we
return CASE_CONFLICT in case we have a conflict with an update in the
same transaction?

It feels awkward, but I guess that's the best thing we can do. We
happily overwrite case-conflicting preexisting refs, so we wouldn't even
see EEXIST in that case. So the only case where we still see that error
is on a D/F conflict, and in that case it makes sense to return
CREATE_EXISTS.

I feel like this should be added as a comment though, as it gives some
important non-obvious context.

Patrick