Re: [PATCH v5 2/7] refs: specify error for regular refs with `old_target`
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Jun 10, 2024, 06:54 UTC
- 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:
Show 24 quoted lines
> 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