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

On 08/21/2012 07:37 PM, Junio C Hamano wrote:

Michael Haggerty [off-list ref] writes:

quoted

quoted

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/