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

From: Karthik Nayak <karthik.188@gmail.com>
Date: Jun 10, 2024, 08:26 UTC
Message-ID: <CAOLa=ZSum=gWbxMyekwjfxqJ38EPTbQ-d4ts-xGxKaiiDgUTNw@mail.gmail.com>
In-Reply-To: <ZmajCzuQftZlmlmS@tanuki>

Patrick Steinhardt <ps@pks.im> writes:

Show 30 quoted lines

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

It makes sense the way you put it, but we also print the 'target', so it
is something like "cannot lock ref 'refs/heads/branch1': expected symref with
target 'refs/heads/master': but is a regular ref". I felt here the colon
divides the error into three segments
1. States that we couldn't lock the file
2. States that we were expecting a symref with a particular target
3. States that the ref in question is actually a regular ref

Having said that, I'm not too bullish on this and happy to change it :)

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

Thanks for all the reviews. Since this is the only change, I'm inclined to leave it as is.

Karthik