Hi Karthik On 26/04/2024 16:24, Karthik Nayak wrote:
From: Karthik Nayak <karthik.188@xxxxxxxxx> The `ref_transaction[_add]_update` functions obtain ref information and flags to create a `ref_update` and add it 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.
s/it/them/
In this commit, let's add the
This commit adds?
required parameters to the function and modify 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. 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. The handling logic of these parameters will be added in consequent commits as we add symref commands to the '--stdin' mode of 'git-update-ref'. Signed-off-by: Karthik Nayak <karthik.188@xxxxxxxxx>
Thanks for updating the documentation, I've left a couple of comments below
diff --git a/refs.h b/refs.h index d278775e08..c792e13a64 100644 --- a/refs.h +++ b/refs.h @@ -648,6 +648,15 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err); * before the update. A copy of this value is made in the * transaction. * + * new_target -- the target reference that the reference will be + * update to point to.
s/update/updated/
This takes precedence over new_oid when set.
I thought it was a bug to set both new_oid and new_target.
If the reference is a regular reference, it will be + * converted to a symbolic reference.
> + *
+ * old_target -- the reference that the reference must be pointing to. + * Will only be taken into account when the reference is a symbolic + * reference.
Does this last sentence mean it is not possible to assert that it is currently a symbolic reference? I thought the point of being able to specify the old value of a ref when updating was to ensure it hadn't changed since it was read. This contradicts the documentation in the next hunk and the description in the commit message.
* flags -- flags affecting the update, passed to * update_ref_lock(). Possible flags: REF_NO_DEREF, * REF_FORCE_CREATE_REFLOG. See those constants for more @@ -713,7 +722,11 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err); * beforehand. The old value is checked after the lock is taken to * prevent races. If the old value doesn't agree with old_oid, the * whole transaction fails. If old_oid is NULL, then the previous - * value is not checked. + * value is not checked. If `old_target` is not NULL, treat the reference + * as a symbolic ref and validate that its target before the update is + * `old_target`. If the `new_target` is not NULL, then the reference + * will be updated to a symbolic ref which targets `new_target`.
This looks good and describes the behavior I'd expected to see. Best Wishes Phillip
