Re: [PATCH v2 1/7] refs: accept symref values in `ref_transaction[_add]_update`

[phillip.wood123@gmail.com]

Hi Karthik

Sorry for the slow response on this

On 19/04/2024 16:47, Karthik Nayak wrote:
>> either way it would be helpful to add some assertions to detect mistakes
>>
>> 	if (old_oid && old_ref)
>> 		BUG("Only one of old_oid and old_ref should be non NULL");
>> 	if (new_oid && new_ref)
>> 		BUG("Only one of new_oid and new_ref should be non NULL");
>>
> 
> I have slightly modified it to:
> 
>   	if (old_oid && !is_null_oid(old_oid) && old_ref)
>   		BUG("Only one of old_oid and old_ref should be non NULL");
>   	if (new_oid && !is_null_oid(new_oid) && new_ref)
>   		BUG("Only one of new_oid and new_ref should be non NULL");
> 
> But I agree, this is needed and have added it.

I'm confused as to why we want to allow old_ref in conjunction with 
old_oid being the null oid. Reading the documentation a null oid means 
"this ref must not exist" and NULL means "I don't care about the current 
state of the ref" so NULL and the null oid are not equivalent.

Best Wishes

Phillip

> 
>>> diff --git a/refs.h b/refs.h
>>> index d278775e08..645fe9fdb8 100644
>>> --- a/refs.h
>>> +++ b/refs.h
>>> @@ -696,13 +696,19 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err);
>>>     */
>>>    #define REF_SKIP_REFNAME_VERIFICATION (1 << 11)
>>>
>>> +/*
>>> + * The reference update is considered to be done on a symbolic reference. This
>>> + * ensures that we verify, delete, create and update the ref correspondingly.
>>> + */
>>> +#define REF_SYMREF_UPDATE (1 << 12)
>>
>> I'm confused as to why we need this as I assumed that we could use the
>> presence of old_ref/new_ref to determine that the caller wants to update
>> symbolic ref. Having this flag means that there are more possibilities
>> to misuse the new API setting this flag but providing NULL for old_ref
>> and new_ref.
>>
> 
> I think I started with this flag but as the direction of the series
> changed and I moved using zero_oid values for deletion or for using the
> verify command, this is not really needed anymore. I just tried removing
> all the code around the flags and fixing up things and all the tests
> still pass. Thanks for brining this up.
> 
> Patrick Steinhardt <ps@pks.im> writes:
>>> I'm confused as to why we need this as I assumed that we could use the
>>> presence of old_ref/new_ref to determine that the caller wants to update
>>> symbolic ref. Having this flag means that there are more possibilities to
>>> misuse the new API setting this flag but providing NULL for old_ref and
>>> new_ref.
>>
>> In my opinion the same comment applies to `REF_HAVE_NEW` and
>> `REF_HAVE_OLD`, which I found to be redundant, as well. Those may make
>> sense in the internals when the object IDs are stored as non-pointers,
>> but queueing ref updates only accepts pointers anyway.
>>
> 
> Yeah like you mentioned, since we're dealing with pointers, checking the
> if its set is enough indication, which doesn't work with the static OID
> values.