From: phillip.wood123@gmail.com Date: Sun, 05 May 2024 15:19:39 GMT Subject: Re: [PATCH v6 1/7] refs: accept symref values in `ref_transaction_update()` Message-ID: In-Reply-To: Hi Karthik On 05/05/2024 16:10, Karthik Nayak wrote: > Hey Phillip, > > Phillip Wood writes: > >> Hi Karthik >> >> On 03/05/2024 13:41, Karthik Nayak wrote: >>> From: Karthik Nayak >>> >>> 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). Thanks for the explanation - it would be good to fix this because the assertions don't catch misuses of ref_transaction_update() at the moment when old_oid points to the null oid and old_target is also set. Best Wishes Phillip > 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]