Re: [PATCH 4/7] refs: extract out refname verification in transactions

From: Christian Couder <christian.couder@gmail.com>
Date: Dec 11, 2024, 09:26 UTC
Message-ID: <CAP8UFD3fZ21TXgtMcppXMHf35qgA-UH=0X9z-xJ456qXyV5=dA@mail.gmail.com>
In-Reply-To: <20241209-320-git-refs-migrate-reflogs-v1-4-d4bc37ee860f@gmail.com>

On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak <karthik.188@gmail.com> wrote:

Show 8 quoted lines

>
> Unless the `REF_SKIP_REFNAME_VERIFICATION` flag is set for an update,
> the refname of the update is verified for:
>
>   - Ensuring it is not a pseudoref.
>   - Checking the refname format.
>
> These checks are also be needed in a following commit where the function

s/are also be needed/will also be needed/

Show 20 quoted lines

> to add reflog updates to the transaction is introduced. Extract the code
> out into a new static function.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  refs.c | 43 ++++++++++++++++++++++++++++---------------
>  1 file changed, 28 insertions(+), 15 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index f003e51c6bf5229bfbce8ce61ffad7cdba0572e0..732c236a3fd0cf324cc172b48d3d54f6dbadf4a4 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1196,6 +1196,29 @@ struct ref_update *ref_transaction_add_update(
>         return update;
>  }
>
> +static int transaction_refname_verification(const char *refname,
> +                                           const struct object_id *new_oid,
> +                                           unsigned int flags,
> +                                           struct strbuf *err)

We have a number of functions named 'xxx_valid()' or 'xxx_ok()' while I couldn't find any 'yyy_verification()' function, so it might be better to name it 'transaction_refname_valid()' or maybe 'transaction_refname_ok()'.

Also I think it should probably return a bool so 1 if the refname is valid and 0 otherwise, unless we have plans in the future to follow different code paths depending on the different ways it is not valid.

> +       ret = transaction_refname_verification(refname, new_oid, flags, err);
> +       if (ret)
> +               return ret;

Then the above could be just:

       if (!transaction_refname_valid(refname, new_oid, flags, err))
               return -1;