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

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Jun 28, 2016, 09:23 UTC
Message-ID: <5772421F.3030100@alum.mit.edu>
In-Reply-To: <576B9081.3020405@alum.mit.edu>

On 06/23/2016 09:32 AM, Michael Haggerty wrote:

Show 10 quoted lines

> [...]
> 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