From: Patrick Steinhardt <ps@pks.im>
Date: Mon, 10 Jun 2024 06:54:03 GMT
Subject: Re: [PATCH v5 2/7] refs: specify error for regular refs with `old_target`
Message-ID: <ZmajCzuQftZlmlmS@tanuki>
In-Reply-To: <20240607133304.2333280-3-knayak@gitlab.com>

On Fri, Jun 07, 2024 at 03:32:59PM +0200, Karthik Nayak wrote:
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 194e74eb4d..fc57c9d220 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2491,14 +2491,16 @@ static int lock_ref_for_update(struct files_ref_store *refs,
>  
>  		/*
>  		 * Even if the ref is a regular ref, if `old_target` is set, we
> -		 * check the referent value. Ideally `old_target` should only
> -		 * be set for symrefs, but we're strict about its usage.
> +		 * fail with an error.
>  		 */
>  		if (update->old_target) {
> -			if (ref_update_check_old_target(referent.buf, update, err)) {
> -				ret = TRANSACTION_GENERIC_ERROR;
> -				goto out;
> -			}
> +			strbuf_addf(err, _("cannot lock ref '%s': "
> +					   "expected symref with target '%s': "
> +					   "but is a regular ref"),
> +				    ref_update_original_update_refname(update),
> +				    update->old_target);
> +			ret = TRANSACTION_GENERIC_ERROR;
> +			goto out;

Shouldn't the second colon be a comma? "expected symref, but is a
regular ref" reads way more natural to me than "expected symref: but is
a regular ref".

Other than that this series looks good to me now, thanks!

Patrick