Hi Karthik
On 05/05/2024 16:10, Karthik Nayak wrote:
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).
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]
