Re: [PATCH v4 1/7] refs: accept symref values in `ref_transaction[_add]_update`

From: Jeff King <peff@peff.net>
Date: Apr 30, 2024, 10:30 UTC
Message-ID: <20240430103002.GD1279403@coredump.intra.peff.net>
In-Reply-To: <b2977495-3b4a-4f2a-85dc-d0568f34f2e9@gmail.com>

On Mon, Apr 29, 2024 at 10:32:58AM +0100, phillip.wood123@gmail.com wrote:

Show 17 quoted lines

> On 26/04/2024 22:15, Jeff King wrote:
> > On Fri, Apr 26, 2024 at 12:31:36PM -0700, Junio C Hamano wrote:
> > 
> > But the more interesting one is a call to refname_is_safe(), added
> > recently by Phillip's 1dbe401563 (show-ref --verify: accept pseudorefs,
> > 2024-02-07). Looking at that commit, the intent was to allow pseudo-refs
> > by loosening the conditional that checked "HEAD" to allow "FOO_BAR" but
> > not "foobar" outside of "refs/". We enforce the all-caps pseudoref
> > syntax in is_refname_safe().
> > 
> > The proper API there is I think check_ref_format() with ALLOW_ONELEVEL.
> 
> ALLOW_ONELEVEL just disables the check that the refname contains a '/' and I
> think it is aimed at checking branch and tag names without a
> refs/{heads,tags} prefix. If we want to move away from using
> refname_is_safe() perhaps we could add an ALLOW_PSEUDOREF flag that only
> allows the name to contain '[A-Z_]' if there is no '/'.

Right, I think that was the original reason for ALLOW_ONELEVEL. But then lots of callers ended up using it because it was the only way to allow "HEAD" or "FETCH_HEAD" to work alongside regular refs.

See the series I posted elsewhere which adds a new flag to cover that case.

-Peff