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

> 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
>