Re: [PATCH v6 4/7] refs: add support for transactional symref updates

[Phillip Wood <phillip.wood123@gmail.com>]

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.

> 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