On 08/21/2012 07:37 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@xxxxxxxxxxxx> 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@xxxxxxxxxxxx
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html