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:
signature.asc
Description: PGP signature
