git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Patrick Steinhardt <ps@pks.im>
Date
Apr 30, 2024, 10:09 UTC
Message-ID
<ZjDDa5PIxPCFzMhq@tanuki>
In-Reply-To
<20240430100145.GB1279403@coredump.intra.peff.net>
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]
Show 20 quoted lines
> > > 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]
Show 23 quoted lines
> > > @@ -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
Previous: Jeff KingNext: Junio C Hamano
Message 7 of 8 in “check_refname_format(): add FULLY_QUALIFIED flag”
  1. 6/8 check_refname_format(): add FULLY_QUALIFIED flagJeff King, Apr 29, 2024
  2. Karthik NayakApr 29, 2024
  3. Jeff KingApr 30, 2024
  4. Patrick SteinhardtApr 30, 2024
  5. Patrick SteinhardtApr 30, 2024
  6. Jeff KingApr 30, 2024
  7. Patrick SteinhardtApr 30, 2024
  8. Junio C HamanoApr 30, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.