On Fri, Apr 26, 2024 at 05:15:29PM -0400, Jeff King wrote:
> On Fri, Apr 26, 2024 at 12:31:36PM -0700, Junio C Hamano wrote:
> 
> > This is an unrelated #leftoverbits tangent, but while trying to find
> > out the reason why "[_add]" in the title looked irritating to me, I
> > noticed that builtin/show-ref.c includes <refs/refs-internal.h>.  I
> > do not know what it uses from the "internal" implementation detail,
> > but the API may have to be cleaned up so that a random "client"
> > caller do not have to do so.
> 
> There are two issues. One is the use of refs_read_raw_ref(), added by
> Patrick's 9080a7f178 (builtin/show-ref: add new mode to check for
> reference existence, 2023-10-31). And it argues there why the regular
> API is unsufficient (mostly because it does not protect errno).
> 
> 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.
> But you shouldn't need to do that, because the refs code should be
> checking the names itself (using check_ref_format() usually, but falling
> back to refname_is_safe() if the ALLOW_BAD_NAME flag is passed).
> 
> And I think there _is_ a bug there. The idea of those two functions is
> that check_ref_format() would allow a subset of what refname_is_safe()
> does. We'd fall back to the latter when deleting, but not otherwise
> allow creation or updates.
> 
> However, it looks like check_ref_format() doesn't enforce the pseudo-ref
> syntax. It will happily resolve this:
> 
>   git rev-parse HEAD >.git/foo
>   git rev-parse foo
> 
> and even update it:
> 
>   git update-ref foo HEAD
> 
> though curiously we will refuse to delete it:
> 
>   $ git update-ref -d foo
>   error: refusing to update ref with bad name 'foo'
> 
> since that sets the ALLOW_BAD_NAME flag!
> 
> IMHO these should _all_ be forbidden, because we only want to allow the
> more limited pseudoref names everywhere (and never mischievous ones like
> "config" or whatever). And once we are doing that, then show-ref has no
> need to check the format. It can just call read_ref() and it either gets
> an answer or doesn't.
> 
> I don't know if that is a #leftoverbit though. It perhaps more
> complicated than that.

Yeah, this is something that I've repeatedly stumbled over myself. If I
remember correctly, the plan was to clean up and consolidate all these
different functions we have for checking ref names such that they become
easier to use and hopefully lead to more consistent behaviour.

In any case, I very much agree that git-update-ref(1) should refuse to
write refs with names that are known-bad. There should probably be an
escape hatch though that at least allows you to _delete_ those, or
otherwise there is no way to remove such a ref in the reftable repo.
Well, except for meddling with the binary format, but I doubt that
anybody would really want to do that.

Patrick