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

[Patrick Steinhardt <ps@pks.im>]

On Wed, Dec 11, 2024 at 10:26:31AM +0100, Christian Couder wrote:
> On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak <karthik.188@gmail.com> wrote:
> > 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;

The only question is whether we want to discern between an invalid
refname and an error. But reading through the code it doesn't seem like
we want to do that as we always return either `-1` on invalid names or
`0` otherwise.

So agreed, this is a good suggestion.

Patrick