Hey Phillip, Phillip Wood <phillip.wood123@xxxxxxxxx> writes: > Hi Karthik > > On 03/05/2024 13:41, Karthik Nayak wrote: >> From: Karthik Nayak <karthik.188@xxxxxxxxx> >> >> 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@xxxxxxxxx > 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]
Attachment:
signature.asc
Description: PGP signature
