Re: [PATCH 1/2] fetch/push: allow refs/*:refs/*
- From
Michael Haggerty <mhagger@alum.mit.edu>
- Date
- Aug 22, 2012, 07:39 UTC
- Message-ID
- <50348C97.4040606@alum.mit.edu>
- In-Reply-To
- <7vpq6kdu31.fsf@alter.siamese.dyndns.org>
On 08/21/2012 07:37 PM, Junio C Hamano wrote:
Show 33 quoted lines
> 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()):
Show 26 quoted lines
> 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/