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]