Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)

[Michael Haggerty <mhagger@alum.mit.edu>]

On 06/23/2016 09:32 AM, Michael Haggerty wrote:
> [...]
> I have to say, I'm still confused about how the old code could have
> worked at all. For quite some time, Git hasn't allowed references with
> weird names like `git-p4-tmp/6` to be created, so how could there be any
> references there that need to be deleted? It seems to me that one of the
> following must be true:
> [...]
> * I'm wrong about Git not allowing references like `git-p4-tmp/6` to
>   be created, in which case I'd like to know what code path allows it
>   so that I can fix it.

I was indeed wrong.

Reference names are vetted by `check_refname_format()` on creation, but
that function knows and enforces nothing about the `refs/` namespace or
the ALL_CAPS convention for top-level references. Moreover, the relevant
caller passes the `REFNAME_ALLOW_ONELEVEL` option, which has semantics
that are fairly useless as far as I can tell. A casual reader might
assume that `REFNAME_ALLOW_ONELEVEL` allows one-level reference only if
they satisfy the special `ALL_CAPS` convention, but in fact it doesn't
enforce the convention. (I suppose that `REFNAME_ALLOW_ONELEVEL` was
meant for checking partial reference names, for example to vet "foo"
that the caller wants to pass to `git branch`, which automatically turns
it into `refs/heads/foo`.)

In summary, `check_refname_format()` is in some ways less strict than
`refname_is_safe()`. For example, it allows reference names like
`git-p4-tmp/6` or even `git-p4-tmp`. With current master:

    $ git update-ref git-p4-tmp HEAD
    $ echo $?
    0
    $ git rev-parse git-p4-tmp
    cf4c2cfe52be5bd973a4838f73a35d3959ce2f43
    $ git update-ref -d git-p4-tmp
    $ echo $?
    0

I don't know what I was thinking. Long ago, when I refactored and
documented check_refname_format(), I realized that the checks are
surprisingly lax and that the `REFNAME_ALLOW_ONELEVEL` option is
misleading. But I was new to the project and wasn't brave or energetic
enough to do something about it. Meanwhile I guess I forgot that it
never got fixed. Commit

d0f810f0 2014-09-03 refs.c: allow listing and deleting badly named refs

, which introduced `refname_is_safe()`, perhaps muddled the situation by
adding a "fallback" check that is not strictly laxer than the main check.

Where does that leave us?

* It is currently possible to create and delete references with
  names that are neither within `refs/` nor ALL_CAPS (though not
  remotely).

* Such references do not participate in reachability checks, so
  they have to be considered semi-broken.

* Users could create chaos (though not remotely) by creating a
  loose "reference" whose name coincides with that of another
  file under `$GIT_DIR`.
  (`git update-ref objects/info/alternates HEAD` anyone?)

* `git-p4` is apparently the only code within the Git project that
  was making use of this questionable freedom.

* Presumably there is user code in the wild that creates and uses
  such references.

I think we need to get this under control, but given that we've allowed
such references (albeit partly broken) for a long time, we probably need
to provide an escape hatch to aid the transition. I'm skeptical that the
mh/split-under-lock patch series is the best place to start such a
campaign. On the other hand, I don't want that patch series to open up
any new avenues to creating references that escape `$GIT_DIR`.

Let me think for a bit about the next step. Input is welcome.

In any case, I think that Lars's patch to `git-p4` is a good thing.

Michael