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

[Jeff King <peff@peff.net>]

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.

-Peff