From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mon, 29 Apr 2024 13:38:55 GMT
Subject: Re: [PATCH v4 1/7] refs: accept symref values in `ref_transaction[_add]_update`
Message-ID: <73632d75-b82d-4853-93fe-3460fdda3325@gmail.com>
In-Reply-To: <20240426152449.228860-2-knayak@gitlab.com>

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?

> 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

> 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