Re: [PATCH 2/2] Restrict ref-like names immediately below $GIT_DIR

[Junio C Hamano <gitster@pobox.com>]

Junio C Hamano <gitster@pobox.com> writes:

> I was trying to summarize this topic for Release Notes.
>
>   Possibly incompatible changes
>   -----------------------------
>
>    * Special refs such as "HEAD", "MERGE_HEAD", and "FETCH_HEAD" are now
>      restricted to names with only uppercase letters and underscore. All
>      other refs must live under refs/ namespace. Earlier, you could
>      confuse git by storing an object name in $GIT_DIR/tmp/junk and say
>      "git show tmp/junk", but this will no longer work.
>
> But noticed that "git update-ref tmp/junk HEAD" does create such a ref
> that won't be recognized, and "git check-ref-format tmp/junk" is happy.
>
> I think we would need to restrict check_ref_format() so that these
> commands (and possibly others, but I think that single function will cover
> pretty much everything) also reject "tmp/junk" immediately below $GIT_DIR
> as a bad string. Otherwise we cannot merge these fixups, which would mean
> we would have to revert the "Clean up refname checks and normalization"
> series, at least the part that started emitting the "warning", which is a
> mess I would rather want to avoid.
>
> Opinions on how to get us out of this mess?

Addendum.

I was digging this further and see fairly large conflicts between the bulk
of "clean up refname checks and normalization" topic and the logic to
avoid the additional warning by tightening the dwimmery.

check_refname_format() has always assumed that it is OK to be called at
any level of substring, and there are many code like this one (example is
from remote.c::get_fetch_map()):

        for (rmp = &ref_map; *rmp; ) {
                if ((*rmp)->peer_ref) {
                        if (check_refname_format((*rmp)->peer_ref->name + 5,
                                REFNAME_ALLOW_ONELEVEL)) {
                                struct ref *ignore = *rmp;
                                error("* Ignoring funny ref '%s' locally",
                                      (*rmp)->peer_ref->name);

This code somehow _knows_ that peer_ref->name begins with "refs/" and that
is the reason it adds 5 to skip that known part. In this particular case,
I think we can simply drop the +5 and allow-onelevel, but there are other
instances of the calls to the function that feeds the rest of the refname
string, skipping leading substring (not necessarily "refs/"), assuming
that any component string is either valid or invalid no matter where it
appears in the full refname. I wouldn't be surprised if some callers do
not even have enough information to tell what the leading substring would
be in the full refname context (e.g. parsing of "master:master" refspec,
relying on the later dwimmery to add refs/heads/ in front, could just
verify that "master" is in good format with allow-onelevel).

The new restriction bolted on to that logic in jc/check-ref-format-fixup
series to work around the new warning in 629cd3a (resolve_ref(): emit
warnings for improperly-formatted references, 2011-09-15) is incompatible
with the assumption, as we would need to check full refname, and treat the
first refname component very differently from other components. It has to
be either "refs" in multi-component refname, or all caps in a one-level
one, but the callers of check_refname_format() are not designed to feed
the full refname to begin with.

I am tempted to revert 629cd3a (resolve_ref(): emit warnings for
improperly-formatted references, 2011-09-15) that started giving the
warnings, and drop the jc/check-ref-format-fixup topic [*1*] altogether,
but that is a short-sighted workaround I would rather want to avoid. It
essentially declares that the "Clean up refname checks" topic was a
failure and did not manage to really clean things up.

A possible alternative might be to leave check_refname_format() and its
callers as they are, introduce check_full_refname() function that knows
the new restriction on top of check-ref-format-fixup, and use that in
lock_ref_sha1(), lock_any_ref_for_update() and is_refname_available()
[*2*]. That way, we can keep the potentially useful "ill-formed contents
in the ref" warning and avoid possible confusion caused by random files
that are directly under $GIT_DIR, which would be far more preferable in
the longer term.

Anybody wants to give it a try?


[Footnote]

*1* The topic is misnamed; it is about fixing dwimmery, not checking.

*2* This currently does not seem to check the well-formedness of the ref
it is creating at all; it should check the "ref", but not "oldref" to
allow renaming a malformed ref to correct earlier mistakes.