Re: [PATCH v6 1/7] refs: accept symref values in `ref_transaction_update()`

> Hey Phillip,
> 
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> Hi Karthik
>>
>> On 03/05/2024 13:41, Karthik Nayak wrote:
>>> From: Karthik Nayak <karthik.188@gmail.com>
>>>
>>> The function `ref_transaction_update()` obtains ref information and
>>> flags to create a `ref_update` and add them to the transaction at hand.
>>>
>>> To extend symref support in transactions, we need to also accept the
>>> old and new ref targets and process it. This commit adds the required
>>> parameters to the function and modifies all call sites.
>>>
>>> The two parameters added are `new_target` and `old_target`. The
>>> `new_target` is used to denote what the reference should point to when
>>> the transaction is applied. Some functions allow this parameter to be
>>> NULL, meaning that the reference is not changed.
>>>
>>> The `old_target` denotes the value the reference must have before the
>>> update. Some functions allow this parameter to be NULL, meaning that the
>>> old value of the reference is not checked.
>>>
>>> We also update the internal function `ref_transaction_add_update()`
>>> similarly to take the two new parameters.
>>
>> The documentation for the new parameters looks good to me now - thanks
>> for updating it. I'm confused about the assertions though as I mentioned
>> in my other message [1].
>>
>> Best Wishes
>>
>> Phillip
>>
>> [1]
>> https://www.lore.kernel.org/git/7ca8c2c4-a9cc-4bec-b13c-95d7854b664b@gmail.com
>>
> 
> Responding here since this is a newer thread.
> 
> This is done because in files-backend we split symref updates (see
> `split_symref_update`) and add a new one for the target reference. Here
> we pass along the update struct. This update struct is memset to 0 and
> this is after the checks we do. So the 'new_oid' here would be set to 0
> (null oid) even if the 'new_target' value is set. This made more sense
> in the earlier set of patches, but probably a diff like this should work
> for this series and can be amended later as needed (in the series which
> adds the symref-* commands).

> diff --git a/refs.c b/refs.c
> index 3645b805c1..20d26da372 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1238,9 +1238,9 @@ struct ref_update *ref_transaction_add_update(
>   	if (transaction->state != REF_TRANSACTION_OPEN)
>   		BUG("update called for transaction that is not open");
> 
> -	if (old_oid && !is_null_oid(old_oid) && old_target)
> +	if (old_oid && old_target)
>   		BUG("only one of old_oid and old_target should be non NULL");
> -	if (new_oid && !is_null_oid(new_oid) && new_target)
> +	if (new_oid && new_target)
>   		BUG("only one of new_oid and new_target should be non NULL");
> 
>   	FLEX_ALLOC_STR(update, refname, refname);
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 3ce260d07d..a718164798 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2328,8 +2328,9 @@ static int split_symref_update(struct ref_update *update,
> 
>   	new_update = ref_transaction_add_update(
>   			transaction, referent, new_flags,
> -			&update->new_oid, &update->old_oid,
> -			NULL, NULL, update->msg);
> +			update->new_target ? NULL : &update->new_oid,
> +			update->old_target ? NULL : &update->old_oid,
> +			update->new_target, update->old_target, update->msg);
> 
>   	new_update->parent_update = update;
> 
> I think it makes sense to make it fool proof and add this, I'll wait for
> more reviews and re-roll in a day or so.
> 
> Thanks for following through.
> 
> [snip]