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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html