Phillip Wood <phillip.wood123@gmail.com> writes:

> Hi Karthik
>
> On 05/05/2024 17:09, Karthik Nayak wrote:
>> Phillip Wood <phillip.wood123@gmail.com> writes:
>>> On 03/05/2024 13:41, Karthik Nayak wrote:
>>>> --- a/refs/reftable-backend.c
>>>> +++ b/refs/reftable-backend.c
>>>> @@ -938,7 +940,22 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
>>>>    		 * individual refs. But the error messages match what the files
>>>>    		 * backend returns, which keeps our tests happy.
>>>>    		 */
>>>> -		if (u->flags & REF_HAVE_OLD && !oideq(&current_oid, &u->old_oid)) {
>>>> +		if (u->old_target) {
>>>> +			if (strcmp(referent.buf, u->old_target)) {
>>>> +				if (!strcmp(referent.buf, ""))
>>>> +					strbuf_addf(err, "verifying symref target: '%s': "
>>>> +						    "reference is missing but expected %s",
>>>> +						    original_update_refname(u),
>>>> +						    u->old_target);
>>>> +				else
>>>> +					strbuf_addf(err, "verifying symref target: '%s': "
>>>> +						    "is at %s but expected %s",
>>>> +						    original_update_refname(u),
>>>> +						    referent.buf, u->old_target);
>>>
>>> The messages in this function differ from the equivalent messages in
>>> check_old_target() from the files backend above. This is potentially
>>> confusing to users, creates more work for translators and makes it hard
>>> to write tests that are independent of the backend. Can we export
>>> check_old_target() so it can be reused here. If not we should reword
>>> these messages to match the other messages all of which talk about not
>>> being able to lock the ref.
>>>
>>
>> This is very intentional, the way the backends work at this point are
>> quite different and while in the files backend, we talk about locking a
>> particular ref.
>
> I agree that the existing messages could be improved - these messages
> are returned when checking the old value of the ref so talking about
> being unable to lock the ref is not helpful as the important information
> is that the old value does not match the expected value. However that is
> not dependent on the backend or on whether the expected value is a
> symref or an oid so it feels a bit random to make these two messages
> different.
>
>> In the reftable backend we do not lock single refs. We
>> lock tables. So keeping it consistent doesn't make sense here.
>
> Where an update is prevented by another process holding a lock I think
> that the granularity of the lock that prevents the ref from being
> updated is not particularly relevant as far as the user is concerned. As
> far as I can see the existing error messages in the reftable backend try
> to be consistent with the messages in the files backend.
>

Well, I agree. But we do have to note, that the files backend always had
`check_old_oid` which has messages along the lines of 'cannot lock ref
...' and now the new `check_old_target` will not be consistent with
those messages. Which is OK, since, like you mentioned, these messages
could be improved.

>> However, we could make the files backend similar to this one, I would be
>> okay doing that.
>
> I would be very happy to see the messages improved for both backends
> when the old value does not match the expected (oid or symref) value. I
> do think we should have consistent error messages in this case that are
> essentially independent of the backend and type of the expected value.
>
> Best Wishes
>
> Phillip

Overall, I think we're reaching the same consensus, i.e. to export this
functionality to a generic function and shared between the both
backends. I will make this change. I think the message used in the
reftable backend along the lines of 'verifying symref target ...' is
verbose yet generic enough, so I'll keep those messages.

Thanks
Karthik