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

> Hi Karthik
>
> I agree with Christian's comments on this patch, I've got a couple of
> additional comments below
>
> On 12/04/2024 10:59, Karthik Nayak wrote:
>> diff --git a/refs.c b/refs.c
>> index 55d2e0b2cb..967c81167e 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -1228,6 +1228,7 @@ struct ref_update *ref_transaction_add_update(
>>   		const char *refname, unsigned int flags,
>>   		const struct object_id *new_oid,
>>   		const struct object_id *old_oid,
>> +		const char *new_ref, const char *old_ref,
>>   		const char *msg)
>>   {
>>   	struct ref_update *update;
>> @@ -1253,6 +1254,7 @@ int ref_transaction_update(struct ref_transaction *transaction,
>>   			   const char *refname,
>>   			   const struct object_id *new_oid,
>>   			   const struct object_id *old_oid,
>> +			   const char *new_ref, const char *old_ref,
>>   			   unsigned int flags, const char *msg,
>>   			   struct strbuf *err)
>>   {
>
> Adding these two new parameters is quite disruptive as all the existing
> callers have to be updated. It makes it easy for callers to misuse this
> function for example by providing old_oid and old_ref (I'm assuming that
> is an error but it is hard to know for sure without any documentation).

> It also makes the calling code harder to read because there are so many
> parameters it is hard to keep track of exactly what is being passed. An
> alternative strategy would be to add a new function that takes a struct
> instead of lots of individual parameters. That would make the calling
> code more readable as it would be clear which struct members are being
> set (see reset.h for an example of this). The approach of adding a
> struct is still prone to setting the wrong combination of options so

> 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");
>

>> 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'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.
>