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
