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

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

On 08/21/2012 07:37 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>>> 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))
>>>  			; /* trash */
>>>  		else if (args.fetch_all &&
>>>  			 (!args.depth || prefixcmp(ref->name, "refs/tags/") )) {
>>
>> I understand that you didn't introduce this code, but it seems like a
>> suspicious combination of conditions:
>>
>>     if ((ref->name starts with "refs/")
>>         and (ref->name has invalid format))
> 
> This protects us from getting contaminated by bogus ref under refs/
> when running "fetch refs/heads/*:refs/remotes/origin/*" no?
> 
> The remote side can also throw phony "I have this object, too, but
> not at a particular ref---this entry is only to let you know I have
> it, so that we can negotiate minimal transfer better" entries that
> are labelled with strings that do not begin with "refs/" and do not
> pass check_refname_format() (and because they are not refs, they do
> not have to pass the test) at us, and we do not want to filter them
> out in this function.  But we do not want anything that is malformed
> under "refs/".

Thanks for the explanation.  I'm trying to dig some more into this so
that I can add some documentation, because this area of the code is
rather obscure.

Here is the loop being discussed, in full (from builtin/fetch-pack.c,
filter_refs()):

> 	for (ref = *refs; ref; ref = next) {
> 		next = ref->next;
> 		if (!memcmp(ref->name, "refs/", 5) &&
> 		    check_refname_format(ref->name, 0))
> 			; /* trash */
> 		else if (args.fetch_all &&
> 			 (!args.depth || prefixcmp(ref->name, "refs/tags/") )) {
> 			*newtail = ref;
> 			ref->next = NULL;
> 			newtail = &ref->next;
> 			continue;
> 		}
> 		else {
> 			int i;
> 			for (i = 0; i < nr_match; i++) {
> 				if (!strcmp(ref->name, match[i])) {
> 					match[i][0] = '\0';
> 					return_refs[i] = ref;
> 					break;
> 				}
> 			}
> 			if (i < nr_match)
> 				continue; /* we will link it later */
> 		}
> 		free(ref);
> 	}

Empirically (determined by instrumenting the code and running the git
test suite):

* The first branch of the if statement is only executed for ref->name of
the form "refs/tags/foo^{}" for various "foo".

* The second branch of the if is *never* executed.

* The third branch is invoked for various reference names under "refs/"
(including oddballs like "refs/for/refs/heads/master", "refs/stash",
"refs/replace/<SHA1>"), and also for "HEAD".

This doesn't quite agree with your explanation, because the phony refs
(at least in this dataset) *do* start with "refs/" and they *are* trashed.

I'll continue to try to figure out this area.  I already found an
apparent memory leak...

Michael

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