Christian Couder <christian.couder@xxxxxxxxx> writes:
> On Fri, Apr 12, 2024 at 11:59 AM Karthik Nayak <karthik.188@xxxxxxxxx> 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 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@xxxxxxxxx>
>
>> 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:
signature.asc
Description: PGP signature
