Phillip Wood <phillip.wood123@gmail.com> writes:

> Hi Karthik
>
> On 26/04/2024 16:24, Karthik Nayak wrote:
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> 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@gmail.com>
>
> 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.