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

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

On 05/05/2012 08:03 AM, Michael Haggerty wrote:
> On 05/05/2012 12:30 AM, Junio C Hamano wrote:
>> There are a handful of places where we call check_refname_format() on a
>> substring after "refs/" of a refname we are going to use, and filter
>> out a
>> valid match with "refs/stash" with such a pathspec. Not sending a stash
>> may arguably be a feature (as stash is inherently a local workflow
>> element), but the code in the transport layer is oblivious to this
>> filtering performed by the lower layer of the code, and complains that
>> the
>> other side did not send all the objects that needs to complete refs/stash
>> at the end, even though the code will not write refs/stash out anyway,
>> and
>> making the whole command fail.
>>
>> This is an attempt to "fix" it by using check_refname_format() on the
>> whole "refs/....." string and allowing refs/stash to be also copied.
>>
>> Signed-off-by: Junio C Hamano<gitster@pobox.com>
>> ---
>>
>> * With this patch:
>>
>> $ git checkout HEAD^0 ;# make sure we are on detached HEAD
>> $ git fetch $somewhere +refs/*:refs/*
>>
>> and
>>
>> victim$ git config receive.denyCurrentBranch warn
>> master$ git push victim +refs/*:refs/*
>>
>> should work.
>>
>> builtin/fetch-pack.c | 2 +-
>> builtin/receive-pack.c | 2 +-
>> remote.c | 2 +-
>> t/t5516-fetch-push.sh | 30 ++++++++++++++++++++++++++++++
>> 4 files changed, 33 insertions(+), 3 deletions(-)
>>
>> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
>> index 6207ecd..a3e3fa3 100644
>> --- a/builtin/fetch-pack.c
>> +++ b/builtin/fetch-pack.c
>> @@ -546,7 +546,7 @@ static void filter_refs(struct ref **refs, int
>> nr_match, char **match)
>> for (ref = *refs; ref; ref = next) {
>> next = ref->next;
>> if (!memcmp(ref->name, "refs/", 5)&&
>> - check_refname_format(ref->name + 5, 0))
>> + check_refname_format(ref->name, 0))
>
> The patch looks fine to me.
>
> 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)". However, now I think
> that the options should be constructed a little differently:
>
> flags==0: Require refname to start with "refs/"
>
> flags==REFNAME_ALLOW_SPECIAL: Also accept single-level ALL_CAPS refnames.
>
> flags==REFNAME_ALLOW_PARTIAL: Don't check the namespace or require '/').
> This could be used for checking partial names like "master" as shorthand
> for "refs/master".
>
> Does this sound reasonable to you?

Oops; I just realized that this particular example doesn't check "starts 
with "refs/" and valid refname" as expected.  In fact it checks "starts 
with "refs/" and INVALID refname".  The proposed REFNAME_FULL option 
would of course be equivalent to the former.  (Why the latter is needed 
here is something that needs a larger investment of time to figure out.)

The other examples that you patched do the expected combination of checks.

Michael

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