[PATCH] make sure parsed wildcard refspec ends with slash

A wildcard refspec is internally parsed into a refspec structure with src
and dst strings.  Many parts of the code assumed that these do not include
the trailing "/*" when matching the wildcard pattern with an actual ref we
see at the remote.  What this meant was that we needed to make sure not
just that the prefix matched, and also that a slash followed the part that
matched.

But a codepath that scans the result from ls-remote and finds matching
refs forgot to check the "matching part must be followed by a slash" rule.
This resulted in "refs/heads/b1" from the remote side to mistakenly match
the source side of "refs/heads/b/*:refs/remotes/b/*" refspec.

Worse, the refspec crafted internally by "git-clone", and a hardcoded
preparsed refspec that is used to implement "git-fetch --tags", violated
this "parsed widcard refspec does not end with slash" rule; simply adding
the "matching part must be followed by a slash" rule then would have
broken codepaths that use these refspecs.

This commit changes the rule to require a trailing slash to parsed
wildcard refspecs.  IOW, "refs/heads/b/*:refs/remotes/b/*" is parsed as
src = "refs/heads/b/" and dst = "refs/remotes/b/".  This allows us to
simplify the matching logic because we only need to do a prefixcmp() to
notice "refs/heads/b/one" matches and "refs/heads/b1" does not.

Signed-off-by: Junio C Hamano <redacted>
---
Junio C Hamano [off-list ref] writes:

I have a nagging suspicion that it might be a simpler and cleaner change
to change parse_refspec_internal() to keep the trailing slash, instead of
dropping it.  Then the check you added is not needed (the trailing slash
guarantees that the pattern matches at the hierarchy boundary), neither
any of the change in this patch.

This is the other variant, and it turns out that I was right.  Among the
64-18 = 46 new lines, 30 are from the new test file.  Two existing
"matching part is followed by '/'" tests are removed.

 remote.c               |   52 +++++++++++++++++++++++++++++++----------------
 t/t5513-fetch-track.sh |   30 +++++++++++++++++++++++++++
 2 files changed, 64 insertions(+), 18 deletions(-)

diff --git a/remote.c b/remote.c
index 0d6020b..f61a3ab 100644
--- a/remote.c
+++ b/remote.c

@@ -427,6 +427,28 @@ static void read_config(void)
 	alias_all_urls();
 }
 
+/*
+ * We need to make sure the tracking branches are well formed, but a
+ * wildcard refspec in "struct refspec" must have a trailing slash. We
+ * temporarily drop the trailing '/' while calling check_ref_format(),
+ * and put it back.  The caller knows that a CHECK_REF_FORMAT_ONELEVEL
+ * error return is Ok for a wildcard refspec.
+ */
+static int verify_refname(char *name, int is_glob)
+{
+	int result, len = -1;
+
+	if (is_glob) {
+		len = strlen(name);
+		assert(name[len - 1] == '/');
+		name[len - 1] = '\0';
+	}
+	result = check_ref_format(name);
+	if (is_glob)
+		name[len - 1] = '/';
+	return result;
+}
+
 static struct refspec *parse_refspec_internal(int nr_refspec, const char **refspec, int fetch, int verify)
 {
 	int i;

@@ -434,11 +456,11 @@ static struct refspec *parse_refspec_internal(int nr_refspec, const char **refsp
 	struct refspec *rs = xcalloc(sizeof(*rs), nr_refspec);
 
 	for (i = 0; i < nr_refspec; i++) {
-		size_t llen, rlen;
+		size_t llen;
 		int is_glob;
 		const char *lhs, *rhs;
 
-		llen = rlen = is_glob = 0;
+		llen = is_glob = 0;
 
 		lhs = refspec[i];
 		if (*lhs == '+') {

@@ -458,12 +480,9 @@ static struct refspec *parse_refspec_internal(int nr_refspec, const char **refsp
 		}
 
 		if (rhs) {
-			rhs++;
-			rlen = strlen(rhs);
+			size_t rlen = strlen(++rhs);
 			is_glob = (2 <= rlen && !strcmp(rhs + rlen - 2, "/*"));
-			if (is_glob)
-				rlen -= 2;
-			rs[i].dst = xstrndup(rhs, rlen);
+			rs[i].dst = xstrndup(rhs, rlen - is_glob);
 		}
 
 		llen = (rhs ? (rhs - lhs - 1) : strlen(lhs));

@@ -471,7 +490,7 @@ static struct refspec *parse_refspec_internal(int nr_refspec, const char **refsp
 			if ((rhs && !is_glob) || (!rhs && fetch))
 				goto invalid;
 			is_glob = 1;
-			llen -= 2;
+			llen--;
 		} else if (rhs && is_glob) {
 			goto invalid;
 		}

@@ -488,7 +507,7 @@ static struct refspec *parse_refspec_internal(int nr_refspec, const char **refsp
 			if (!*rs[i].src)
 				; /* empty is ok */
 			else {
-				st = check_ref_format(rs[i].src);
+				st = verify_refname(rs[i].src, is_glob);
 				if (st && st != CHECK_REF_FORMAT_ONELEVEL)
 					goto invalid;
 			}

@@ -503,7 +522,7 @@ static struct refspec *parse_refspec_internal(int nr_refspec, const char **refsp
 			} else if (!*rs[i].dst) {
 				; /* ok */
 			} else {
-				st = check_ref_format(rs[i].dst);
+				st = verify_refname(rs[i].dst, is_glob);
 				if (st && st != CHECK_REF_FORMAT_ONELEVEL)
 					goto invalid;
 			}

@@ -518,7 +537,7 @@ static struct refspec *parse_refspec_internal(int nr_refspec, const char **refsp
 			if (!*rs[i].src)
 				; /* empty is ok */
 			else if (is_glob) {
-				st = check_ref_format(rs[i].src);
+				st = verify_refname(rs[i].src, is_glob);
 				if (st && st != CHECK_REF_FORMAT_ONELEVEL)
 					goto invalid;
 			}

@@ -532,13 +551,13 @@ static struct refspec *parse_refspec_internal(int nr_refspec, const char **refsp
 			 * - otherwise it must be a valid looking ref.
 			 */
 			if (!rs[i].dst) {
-				st = check_ref_format(rs[i].src);
+				st = verify_refname(rs[i].src, is_glob);
 				if (st && st != CHECK_REF_FORMAT_ONELEVEL)
 					goto invalid;
 			} else if (!*rs[i].dst) {
 				goto invalid;
 			} else {
-				st = check_ref_format(rs[i].dst);
+				st = verify_refname(rs[i].dst, is_glob);
 				if (st && st != CHECK_REF_FORMAT_ONELEVEL)
 					goto invalid;
 			}

@@ -687,8 +706,7 @@ int remote_find_tracking(struct remote *remote, struct refspec *refspec)
 		if (!fetch->dst)
 			continue;
 		if (fetch->pattern) {
-			if (!prefixcmp(needle, key) &&
-			    needle[strlen(key)] == '/') {
+			if (!prefixcmp(needle, key)) {
 				*result = xmalloc(strlen(value) +
 						  strlen(needle) -
 						  strlen(key) + 1);

@@ -966,9 +984,7 @@ static const struct refspec *check_pattern_match(const struct refspec *rs,
 			continue;
 		}
 
-		if (rs[i].pattern &&
-		    !prefixcmp(src->name, rs[i].src) &&
-		    src->name[strlen(rs[i].src)] == '/')
+		if (rs[i].pattern && !prefixcmp(src->name, rs[i].src))
 			return rs + i;
 	}
 	if (matching_refs != -1)

diff --git a/t/t5513-fetch-track.sh b/t/t5513-fetch-track.sh
new file mode 100755
index 0000000..9e74862
--- /dev/null
+++ b/t/t5513-fetch-track.sh

@@ -0,0 +1,30 @@
+#!/bin/sh
+
+test_description='fetch follows remote tracking branches correctly'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	>file &&
+	git add . &&
+	test_tick &&
+	git commit -m Initial &&
+	git branch b-0 &&
+	git branch b1 &&
+	git branch b/one &&
+	test_create_repo other &&
+	(
+		cd other &&
+		git config remote.origin.url .. &&
+		git config remote.origin.fetch "+refs/heads/b/*:refs/remotes/b/*"
+	)
+'
+
+test_expect_success fetch '
+	(
+		cd other && git fetch origin &&
+		test "$(git for-each-ref --format="%(refname)")" = refs/remotes/b/one
+	)
+'
+
+test_done

On Sat, 26 Jul 2008, Junio C Hamano wrote:

A wildcard refspec is internally parsed into a refspec structure with src
and dst strings.  Many parts of the code assumed that these do not include
the trailing "/*" when matching the wildcard pattern with an actual ref we
see at the remote.  What this meant was that we needed to make sure not
just that the prefix matched, and also that a slash followed the part that
matched.

But a codepath that scans the result from ls-remote and finds matching
refs forgot to check the "matching part must be followed by a slash" rule.
This resulted in "refs/heads/b1" from the remote side to mistakenly match
the source side of "refs/heads/b/*:refs/remotes/b/*" refspec.

Worse, the refspec crafted internally by "git-clone", and a hardcoded
preparsed refspec that is used to implement "git-fetch --tags", violated
this "parsed widcard refspec does not end with slash" rule; simply adding
the "matching part must be followed by a slash" rule then would have
broken codepaths that use these refspecs.

This commit changes the rule to require a trailing slash to parsed
wildcard refspecs.  IOW, "refs/heads/b/*:refs/remotes/b/*" is parsed as
src = "refs/heads/b/" and dst = "refs/remotes/b/".  This allows us to
simplify the matching logic because we only need to do a prefixcmp() to
notice "refs/heads/b/one" matches and "refs/heads/b1" does not.

Signed-off-by: Junio C Hamano <redacted>
---
Junio C Hamano [off-list ref] writes:

quoted

I have a nagging suspicion that it might be a simpler and cleaner change
to change parse_refspec_internal() to keep the trailing slash, instead of
dropping it.  Then the check you added is not needed (the trailing slash
guarantees that the pattern matches at the hierarchy boundary), neither
any of the change in this patch.

This is the other variant, and it turns out that I was right.  Among the
64-18 = 46 new lines, 30 are from the new test file.  Two existing
"matching part is followed by '/'" tests are removed.

Yeah, the first version of this code included the '/', by virtue of having 
the '/' not be required in the refspec (i.e., you could have 
"refs/heads/b*"); when you told me that refspecs needed to have a '/' 
before the '*', I thought that it would be easiest to not include the '/', 
since it was redundant, but, in retrospect, I'm not too surprised that it 
simplifies things to include it.

Acked-by: Daniel Barkalow <redacted>

Maybe check_ref_format() ought to have a return for "this is valid as a 
directory rather than a single ref", and that would be allowed with globs? 
I'm not too fond of temporarily changing strings for testing. Also, this 
design (while it matches the current code), means that check_ref_format() 
sees one fewer level than the refs that match will actually have, which is 
a little confusing.

	-Daniel
*This .sig left intentionally blank*

On Sat, Jul 26, 2008 at 11:15:51PM -0700, Junio C Hamano wrote:

quoted

I have a nagging suspicion that it might be a simpler and cleaner change
to change parse_refspec_internal() to keep the trailing slash, instead of
dropping it.  Then the check you added is not needed (the trailing slash
guarantees that the pattern matches at the hierarchy boundary), neither
any of the change in this patch.

This is the other variant, and it turns out that I was right.  Among the
64-18 = 46 new lines, 30 are from the new test file.  Two existing
"matching part is followed by '/'" tests are removed.

Looks like you have already applied it, but I will chime in that of the
two, I think this is the more sensible change. Stripping the '/' felt
like a loss of information to me. IIRC, when "support only /*" was
discussed, the thinking was "let's keep it tight now, and we can loosen
it later." Keeping the '/' means that the code is much more sane if
other globbing rules come in the future.

-Peff