Re: [PATCH v2 1/7] refs: accept symref values in `ref_transaction[_add]_update`

[Karthik Nayak <karthik.188@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5373 bytes --]

Christian Couder <christian.couder@gmail.com> writes:

> On Fri, Apr 12, 2024 at 11:59 AM Karthik Nayak <karthik.188@gmail.com> 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 values and process it. In this commit, let's add the
>> required paramaters to the function and modify all call sites.
>
> s/paramaters/parameters/
>
>> The two paramaters added are `new_ref` and `old_ref`. The `new_ref` is
>
> s/paramaters/parameters/
>

Thanks, will fix both.

>> 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, or `""`, meaning that the reference should
>> be deleted.
>
>> The `old_ref` denotes the value of that the reference must have before
>
> s/the value of that the reference/the value the reference/
>

Will change.

>> the update. Some functions allow this parameter to be NULL, meaning that
>> the old value of the reference is not checked, or `""`, meaning that the
>> reference must not exist before the update. A copy of this value is made
>> in the transaction.
>>
>> The handling logic of these parameters will be added in consequent
>> commits as we implement symref-{create, update, delete, verify}.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>
>> diff --git a/refs.h b/refs.h
>> index d278775e08..645fe9fdb8 100644
>> --- a/refs.h
>> +++ b/refs.h
>
> There is the following big comment in this file:
>
> /*
>  * Reference transaction updates
>  *
>  * The following four functions add a reference check or update to a
>  * ref_transaction.  They have some common similar parameters:
>  *
>  *     transaction -- a pointer to an open ref_transaction, obtained
>  *         from ref_transaction_begin().
>  *
>  *     refname -- the name of the reference to be affected.
>  *
>  *     new_oid -- the object ID that should be set to be the new value
>  *         of the reference. Some functions allow this parameter to be
>  *         NULL, meaning that the reference is not changed, or
>  *         null_oid, meaning that the reference should be deleted. A
>  *         copy of this value is made in the transaction.
>  *
>  *     old_oid -- the object ID that 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,
>  *         or null_oid, meaning that the reference must not exist
>  *         before the update. A copy of this value is made in the
>  *         transaction.
>  *
>  *     flags -- flags affecting the update, passed to
>  *         update_ref_lock(). Possible flags: REF_NO_DEREF,
>  *         REF_FORCE_CREATE_REFLOG. See those constants for more
>  *         information.
>  *
>  *     msg -- a message describing the change (for the reflog).
>  *
>  *     err -- a strbuf for receiving a description of any error that
>  *         might have occurred.
>  *
>  * The functions make internal copies of refname and msg, so the
>  * caller retains ownership of these parameters.
>  *
>  * The functions return 0 on success and non-zero on failure. A
>  * failure means that the transaction as a whole has failed and needs
>  * to be rolled back.
>  */
>
> I would expect the patch to update this comment.
>

Since this patch doesn't use this value, I think its best to modify
this in each patch as we start using it, I'll do that.

>> @@ -722,6 +728,7 @@ int ref_transaction_update(struct ref_transaction *transaction,
>>                            const char *refname,
>>                            const struct object_id *new_oid,
>>                            const struct object_id *old_oid,
>> +                          const char *new_ref, const char *old_ref,
>>                            unsigned int flags, const char *msg,
>>                            struct strbuf *err);
>
> The patch might also want to update the comment just above the
> ref_transaction_update() declaration as it is changing what the
> function can (or will be able to) do.
>

Agreed, same as above, will modify in each patch. Also will add a
comment in the commit.

>> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
>> index 56641aa57a..4c5fe02687 100644
>> --- a/refs/refs-internal.h
>> +++ b/refs/refs-internal.h
>> @@ -124,6 +124,19 @@ struct ref_update {
>>          */
>>         struct object_id old_oid;
>>
>> +       /*
>> +        * If (flags & REF_SYMREF_UPDATE), set the reference to this
>> +        * value (or delete it, if `new_ref` is an empty string).
>
> What if this value is NULL?
>
>> +        */
>> +       const char *new_ref;
>> +
>> +       /*
>> +        * If (type & REF_SYMREF_UPDATE), check that the reference
>> +        * previously had this value (or didn't previously exist,
>> +        * if `old_ref` is an empty string).
>
> What if this value is NULL?
>

Well we ignore NULL values, but I see that the documentation is lacking,
will update.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]