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

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

On 10/19/2011 08:19 AM, Junio C Hamano wrote:
> 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?

I think that the refs/-or-ALL_CAPS test fits most naturally in
check_refname_format(), controlled by an option flag.

I'm starting by building parts of the solution, namely something like:

* Add an option REFNAME_ALLOW_DWIM which causes check_refname_format()
to accept reference names that *could* be dwimmed into a valid refname.
 Specifically, when this option is *not* specified, then the option must
start with "refs/" or be ALL_CAPS.  When it *is* specified, the behavior
will be much like the current behavior when ALLOW_ONELEVEL is specified
(in fact, the new option might make ALLOW_ONELEVEL obsolete).

* Add a new function parse_refname_prefix(str, len, flags) which tries
to read a refname from the front of str (of length len, not necessarily
NUL-terminated) and returns the length of the part that could be used.
This function will support the same flag argument as
check_refname_format() (in fact the latter would become a trivial
wrapper of the new function).  I expect that this function will be
useful for dwimmery.

Hopefully I'll have patches before the end of the (Berlin) day.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/