Patrick Steinhardt <ps@xxxxxx> writes:
> 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
Attachment:
signature.asc
Description: PGP signature
