Re: [PATCH] fetch/push: allow refs/*:refs/*

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

On 05/07/2012 06:24 PM, Junio C Hamano wrote:
> Michael Haggerty<mhagger@alum.mit.edu>  writes:
>
>> This combination "!memcmp(ref->name, "refs/", 5)&&
>> check_refname_format(ref->name, 0)" is the reason that I suggested
>> adding a REFNAME_FULL option [1], in which case it could be written
>> "check_refname_format(ref->name, REFNAME_FULL)".
>
> I personally think that check-refname-format should lose its second
> parameter and always check for a concrete full refs, so that we can
> tighten the current allow-onelevel case a bit further (i.e. ".git/HEAD" is
> OK at the root level, but ".git/refs/heads/HEAD" is asking for trouble,
> and ".git/config" is downright confusing. The function does not get a good
> enough clue by only ONELEVEL).  That would mean REFNAME_FULL will not be
> necessary---it should be the only mode of operation, and the callers need
> to be adjusted accordingly.
>
> We should also introduce another function check-refspec-half-format to be
> used for the checks for left and right halves of refspec ("refs/heads/*"
> is OK but "refs/heads*" is not, perhaps).
>
> A single function trying to do both and not doing a good job in either
> case is not a pretty picture.

Whether the functionality is presented via one function with multiple 
options or multiple functions with no options is of secondary 
importance.  Let's first worry about the desired functionality, since 
even that is not clear.


There's a long weekend coming up in Germany so hopefully I'll have 
another increment of time to work on this.  I'd like to get feedback on 
the requirements.  First let me present a summary of what I know about 
refnames in git.


There are a few categories of refname that come up in the git code:

* Full refnames like "refs/foo" or "refs/bar/baz" (starting with "refs/").

* Top-level special ALL_CAPS reference names like "MERGE_HEAD".

* Refspec-style patterns with wildcards, like "refs/heads/*" or 
"refs/foo/*/bar".

* Short branch names with an implied "refs/heads/" prefix omitted; e.g., 
"master" meaning "refs/heads/master".

* Short tag names with an implied "refs/tags/" prefix omitted; e.g., 
"v1.2.3" meaning "refs/tags/v1.2.3".

* Maybe others...?


Currently, I believe that code usually handles the short branch/tag 
names via one of two mechanisms:

* Explicitly prepend "refs/heads/" or "refs/tags/" to the short name to 
make the corresponding full name, then work exclusively with the full name.

* Use the ref_rev_parse_rules based functions like dwim_ref() to guess 
which prefix can be applied to a short refname to make it into a full 
refname, then work exclusively with the full name.

If code will continue to work this way, then there might be no use for a 
function that can check short refnames; one would always convert the 
short refname into a full refname and check the latter.  On the other 
hand, a short refname check might do special things like *reject* a name 
that starts with "refs/".


In addition to the strict rules governing refnames, there are other 
policies that are enforced at varying levels of strictness or that might 
be helpful in the future; for example,

* Refnames either have to start with "refs/" or have to be ALL_CAPS.

* Refnames cannot be directly under "refs/" but should be in a namespace 
like "refs/heads/".  (This is implied by the old check-ref-format 
documentation:

> . They must contain at least one `/`. This enforces the presence of a
>   category like `heads/`, `tags/` etc. but the actual names are not
>   restricted.

This sentence also implies that check-ref-format was intended to be used 
for refnames shortened by having the "refs/" prefix omitted, which is 
curious.)

* Unlikely refnames like "refs/heads/HEAD" could be handled with 
suspicion, as you suggest.  I would also nominate constructs like 
"refs/heads/refs/heads/mybranch", which I've seen cause confusion at $WORK.


In terms of implementation:

* We want to distinguish between what is acceptable for new refnames 
versus refnames that already exist in the local repository and/or 
refnames received from a remote repo.  So there should be some kind of 
"REFNAME_RELAXED" option.

* It is important for the "old" check_refname_format() and the "new" one 
to coexist during the transition.  This is simply because I don't want 
to have to analyze and rewrite all ~30 callers of check_refname_format() 
in one big bang.  I suggest retaining a copy as 
check_refname_format_legacy() that is deprecated and removed after all 
callers have been rewritten.

* The exact current behavior of "git check-ref-format" will probably not 
map well to the new check_refname_format(), and is anyway inadequate and 
somewhat inconsistent.  We need to decide how strict we need to be about 
backwards-compatibility of this command.

* I've been using "git check-ref-format" for testing 
check_refname_format().  But I think it would be a bad idea to expose 
all of the check_refname_format() options and variants in an installed 
plumbing command.  Therefore I think that there should be a program that 
can be used to call check_refname_format() with all of its option 
combinations to be used only for testing purposes.

Michael

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