Re: [PATCH 4/4] remote: check branch names

[Patrick Steinhardt <ps@pks.im>]

On Wed, Sep 11, 2024 at 10:03:26AM -0700, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> 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