Re: [PATCH v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic

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

On 08/20/2014 11:47 PM, Ronnie Sahlberg wrote:
> [...]
> Since we already display broken/unresolvable refs, I think the most
> consistent path is to also allow showing the refs broken/illegal-names
> too in the list. (when DO_FOR_EACH_INCLUDE_BROKEN is specified)
> Of course, an end user could fix this by deleting the file but since
> it is easy to add the special casing to 'git branch -D' to handle this
> case I think this would be more userfriendly since then the user can
> use git branch -D regardless of the reason why the ref is broken.

My concern with this idea is that some code relies on at least some of
the reference name constraints for its proper functioning; for example,

* The ref caching code would likely be confused by ill-formed refnames
like "refs/heads//foo" or "/refs/heads/foo" or "refs/heads/foo/".  (I
understand that such references cannot exist as loose refs, but they
could be represented in the packed-refs file.)

* Any code that might try to read or write a loose reference would
likely be confused by "refs/heads//foo" or "refs/heads/./foo" or
"refs/heads/../foo" or "/refs/heads/foo" or "refs/heads/foo/".  On
Windows there might also be problems with "refs/heads\foo" or
"d:refs/heads/foo" or "prn:refs/heads/foo" or "//refs/heads/foo".

* The locking code could easily be confused by a reference named
"refs/heads/foo.lock".

So to the extent that we loosen the checks on refnames when they are
read, we would have to re-vet any code that touches them to make sure
that it doesn't break in a horrible (and possibly security-compromising)
way.  This is why I would prefer to quarantine broken reference names in
the smallest possible part of the code.

I *think* that the biggest problems would be related to reference names
that do not map straightforwardly to relative filenames, so an
alternative would be to do some minimal checks in any case, but make it
possible to turn off the stricter checks (those that mostly exist to
make reference expression parsing possible) when necessary.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu