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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 19, 2011, 06:19 UTC
Message-ID: <7vty75ec54.fsf@alter.siamese.dyndns.org>
In-Reply-To: <7v39epft32.fsf@alter.siamese.dyndns.org>

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

Show 23 quoted lines

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