Phillip Wood <phillip.wood123@xxxxxxxxx> writes:
> Hi Karthik
>
> On 05/05/2024 17:09, Karthik Nayak wrote:
>> Phillip Wood <phillip.wood123@xxxxxxxxx> 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(¤t_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
Attachment:
signature.asc
Description: PGP signature
