On Wed, Sep 11, 2024 at 10:03:26AM -0700, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
> > +static int check_branch_names(const char **branches)
> > +{
> > + int ret = 0;
> > +
> > + for (const char **b = branches; *b; b++) {
> > + if (check_refname_format(*b, REFNAME_ALLOW_ONELEVEL |
> > + REFNAME_REFSPEC_PATTERN))
> > + ret = error(_("invalid branch name '%s'"), *b);
> > + }
> > +
> > + return ret;
> > +}
>
> This implementation is inconsistent with what "git branch new HEAD"
> uses to check the validity of "new", which is in this call chain:
>
> builtin/branch.c:cmd_branch()
> -> branch.c:create_branch()
> -> branch.c:validate_new_branchname()
> -> branch.c:validate_branchname()
> -> object-name.c:strbuf_check_branch_ref()
>
> At least, we should prepend "refs/heads/" to *b, so that we can
> reject "refs/heads/HEAD". The authoritative logic in the above
> however may further evolve, and we need to make sure that these two
> checks from drifting away from each other over time. We probably
> should refactor the leaf function in the above call chain so that
> both places can use it (the main difference is that you allow '*' in
> yours when calling check_refname_format()).
>
> Side note: we *should* lose "strbuf_" from its name, as it is
> not about string manipulation but the "strbuf'-ness
> of the function is merely that as the side effect of
> checking it computes a full refname and it happens to
> use strbuf as a mechanism to return it.
>
> Something like the patch attached at the end.
Agreed. It's also kind of curious that the function lives in
"object-name.c" and not in "refs.c".
Patrick
