Phillip Wood <phillip.wood123@xxxxxxxxx> writes: > 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? > Thanks, will add both the above. >> 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 > Thanks for the review. >> 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. > Yup. fixed both of these. > >> 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. > I see how this is vague, I was trying to imply that the old_target is used to check a symref's old_value. But actually, if this is set and the ref is a regular ref, we do fail the check. So this is wrong. Let me just strip the last time.
Attachment:
signature.asc
Description: PGP signature
