Re: [PATCH v8 39/44] refs.c: move the check for valid refname to lock_ref_sha1_basic

On Wed, May 21, 2014 at 6:42 PM, Jonathan Nieder [off-list ref] wrote:

Ronnie Sahlberg wrote:

quoted

--- a/refs.c
+++ b/refs.c

@@ -2044,6 +2044,9 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
      int missing = 0;
      int attempts_remaining = 3;

+     if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
+             return NULL;

Are we sure that no callers are locking a poorly named ref
as preparation for repairing it?

To see what works already in that vein, I tried:

        $ git rev-parse HEAD >.git/refs/heads/foo..bar
        $ git branch -m foo..bar something-saner
        fatal: Invalid branch name: 'foo..bar'

"git branch -m" has an explicit codepath ("recovery = 1;") to handle
this case, but it looks like it was not well tested and regressed in
v1.7.8-rc0~19^2~7 (resolve_ref(): verify that the input refname has
the right format, 2011-09-15).

Is what the recovery codepath of branch -m does misguided?  One
school of thought would be that people with malformed refs are
responsible for recovering using low-level tools like "mv" and "vi"
instead of normal git commands since normal git commands should never
have created such a bad situation.  Another school of thought would
assert that

 * git commands can have bugs
 * the format checked by check_refname_format() keeps getting stricter
   over time

which means it's nice when people can recover with 'update-ref -d'
or 'branch -m'.  It's not obvious to me what the right thing to do
here is (maybe a special flag to be attached to a ref update during
recovery?).

I don't think we should allow external caller any other way to
access/modify refs than through
the transaction api. This may make it awkward to handle cases such as
foo/../bar was created  and how would you now delete it ?

The exception to this I think would be 'git fsck'. We could allow fsck
to have low level access to the backing store and allow it to access
the files directly,
or allow fsck to set magic flags that disable various checks.

Hope that helps,
Jonathan