Re: [PATCH v4 2/7] refs: specify error for regular refs with `old_target`

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2559 bytes --]

On Wed, Jun 05, 2024 at 12:29:53PM +0200, Karthik Nayak wrote:
> From: Karthik Nayak <karthik.188@gmail.com>
> 
> When a regular reference update contains `old_target` set, we call the

s/contains/has its/

> `ref_update_check_old_target` function to check the referent value. But
> for regular refs we know that the referent value is not set and this

This is fairly technical and focussed on the implementation. Can we
maybe talk more about intent ("expected a symref, but is a direct ref")
rather than the exact implementation to make the commit message a bit
easier to understand for folks?

> simply raises a generic error which says nothing about this being a
> regular ref. Instead let's raise a more specific error when a regular
> ref update contains `old_target`.

It might be helpful to include before/after in this commit message to
show the change. Even better would be a test of course, but I think we
cannot add one at this point in time yet.

> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  refs/files-backend.c    | 13 +++++++------
>  refs/reftable-backend.c |  9 +++++++++
>  2 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 194e74eb4d..f516d4d82f 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2491,14 +2491,15 @@ 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 update regular ref: '%s': "
> +					   "symref target '%s' set"),
> +				    ref_update_original_update_refname(update),
> +				    update->old_target);
> +			ret = TRANSACTION_GENERIC_ERROR;
> +			goto out;

I feel like these error messages are somewhat technical. If I were
reading it as a user, I don't think I'd understand what it is trying to
tell me. How about:

        strbuf_addf(err, _("cannot lock ref '%s': %s",
                           "expected symref but is a direct ref"));

This also matches the other error messages we have in this function more
closely. The same is true for the equivalent in the reftable backend.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]