Re: [PATCH 6/8] check_refname_format(): add FULLY_QUALIFIED flag

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2648 bytes --]

On Tue, Apr 30, 2024 at 06:01:45AM -0400, Jeff King wrote:
> On Tue, Apr 30, 2024 at 06:54:04AM +0200, Patrick Steinhardt wrote:
> > On Mon, Apr 29, 2024 at 04:33:25AM -0400, Jeff King wrote:
[snip]
> > > The whole ALLOW_ONELEVEL thing is a long-standing confusion, and
> > > unfortunately has made it into the hands of users via "git
> > > check-ref-format --allow-onelevel". So I think it is there to stay.
> > > Possibly we should expose this new feature as --fully-qualified or
> > > similar.
> > 
> > Hm, that's really too bad. I wonder whether we should eventually start
> > to deprecate `--allow-onelevel` in favor of `--fully-qualified`. We
> > would continue to accept the flag, but remove it from our documentation
> > such that scripts start to move over. Then some day, we may replace
> > `ALLOW_ONELEVEL` with something like `ALLOW_ROOT_REF` that allows refs
> > in the root directory while honoring `is_pseudoref_syntax()`.
> 
> I don't know if we could ever get rid of --allow-onelevel. If you want
> to check a branch name, say, the replacement for it is to ask about
> "refs/heads/$name". But sometimes you don't actually know how the short
> name is going to be used, but you want to make sure it's syntactically
> valid. E.g., validating a refspec may involve a name like "main" on its
> own. I suspect it would be OK in practice to just give it an arbitrary
> "refs/foo/$main", but that feels kind of hacky.

Ah, fair enough.

[snip]
> > > @@ -322,8 +331,11 @@ static int check_or_sanitize_refname(const char *refname, int flags,
> > >  		else
> > >  			return -1;
> > >  	}
> > > -	if (!(flags & REFNAME_ALLOW_ONELEVEL) && component_count < 2)
> > > +
> > > +	if (!(flags & (REFNAME_ALLOW_ONELEVEL | REFNAME_FULLY_QUALIFIED)) &&
> > > +	    component_count < 2)
> > >  		return -1; /* Refname has only one component. */
> > > +
> > 
> > I first thought that we don't have to handle REFNAME_FULLY_QUALIFIED
> > here because the above should already handle it. But we can of course
> > have a single component, only, when the ref is "refs/".
> 
> I hadn't really considered that case. The reason we have to handle
> FULLY_QUALIFIED here is that without it, "FETCH_HEAD" (or for that
> matter "HEAD") is forbidden as having only a single component. The
> earlier hunk only rejects bad things, so we still end up in this code.
> 
> I think that "refs/" is forbidden both before and after my patch because
> it's invalid to have a zero-length component (so "foo//bar" is
> forbidden, but so is "foo/" because of the empty component on the end).a

Makes sense, thanks.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]