Re: [PATCH] remote.c: Fix overtight refspec validation

Subsystems: the rest

2 messages, 2 authors, 2016-06-15 · open the first message on its own page

Re: [PATCH] remote.c: Fix overtight refspec validation

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:44:25

Junio C Hamano [off-list ref] writes:

Especially that "refs/tags/*" is sad in that it is leaking an internal
implementation detail.  I do not think the original code ever used
wildcards on the push side, and it probably was a good idea to allow
wildcards when the code was rewritten.

Having thought about this a bit more, I think this patch would be more
useful.  Please discard the previous patch to builtin-push.c and replace
with this patch.

Now it allows you to say:

	[remote "neigh"]
        	url = ../neighbour
                push = refs/tags/*

to propagate all tags one-to-one, without having to say
"refs/tags/*:refs/tags/*".

This however has unintended side effect of allowing 

	[remote "bour"]
        	url = ../neighbour
                fetch = refs/heads/*

at the syntax level.  I do not know offhand the fetch backends are
prepared to deal with such wildcard patterns.

Daniel?

---
 remote.c |   18 +++++++++++-------
 1 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/remote.c b/remote.c
index 40ed246..c39d831 100644
--- a/remote.c
+++ b/remote.c

@@ -417,17 +417,21 @@ static struct refspec *parse_refspec_internal(int nr_refspec, const char **refsp
 			rhs++;
 			rlen = strlen(rhs);
 			is_glob = (2 <= rlen && !strcmp(rhs + rlen - 2, "/*"));
-			rs[i].dst = xstrndup(rhs, rlen - is_glob * 2);
+			if (is_glob)
+				rlen -= 2;
+			rs[i].dst = xstrndup(rhs, rlen);
 		}
 
 		llen = (rhs ? (rhs - lhs - 1) : strlen(lhs));
-		if (is_glob != (2 <= llen && !memcmp(lhs + llen - 2, "/*", 2)))
-			goto invalid;
-
-		if (is_glob) {
+		if (2 <= llen && !memcmp(lhs + llen - 2, "/*", 2)) {
+			if (rhs && !is_glob)
+				goto invalid;
+			is_glob = 1;
 			llen -= 2;
-			rlen -= 2;
+		} else if (rhs && is_glob) {
+			goto invalid;
 		}
+
 		rs[i].pattern = is_glob;
 		rs[i].src = xstrndup(lhs, llen);

@@ -446,7 +450,7 @@ static struct refspec *parse_refspec_internal(int nr_refspec, const char **refsp
 			}
 			/*
 			 * RHS
-			 * - missing is allowed.
+			 * - missing is ok, and is same as empty.
 			 * - empty is ok; it means not to store.
 			 * - otherwise it must be a valid looking ref.
 			 */

Re: [PATCH] remote.c: Fix overtight refspec validation

From: Daniel Barkalow <hidden>
Date: 2016-06-15 22:44:25

On Tue, 25 Mar 2008, Junio C Hamano wrote:

This however has unintended side effect of allowing 

	[remote "bour"]
        	url = ../neighbour
                fetch = refs/heads/*

at the syntax level.  I do not know offhand the fetch backends are
prepared to deal with such wildcard patterns.

Daniel?

It's not a matter of the backends, which don't implement any of the 
control flow in the fetch direction; it's get_expanded_map(), which needs 
to be told that you can have something match a pattern but not have a 
local tracking ref.

OTOH, the only use for such a pattern is an octopus merge of whatever 
branches a remote happens to have, right? I remember thinking this was a 
non-useful refspec when I was dealing with the fetch code (and then 
forgetting that it was useful for push). It might be better to just 
disallow it in the direction-specific semantic checks.

Here's the patch to make it work, anyway:
----

commit 6a8bcb917e1aa9b3c972f14f618ab573e457ebee
Author: Daniel Barkalow [off-list ref]
Date:   Wed Mar 26 01:39:07 2008 -0400

    Support fetching refspecs like "refs/heads/*"
    
    Signed-off-by: Daniel Barkalow [off-list ref]

diff --git a/remote.c b/remote.c
index a027bca..f8f4b34 100644
--- a/remote.c
+++ b/remote.c

@@ -998,22 +998,24 @@ static struct ref *get_expanded_map(const struct ref *remote_refs,
 	struct ref **tail = &ret;
 
 	int remote_prefix_len = strlen(refspec->src);
-	int local_prefix_len = strlen(refspec->dst);
+	int local_prefix_len = refspec->dst ? strlen(refspec->dst) : 0;
 
 	for (ref = remote_refs; ref; ref = ref->next) {
 		if (strchr(ref->name, '^'))
 			continue; /* a dereference item */
 		if (!prefixcmp(ref->name, refspec->src)) {
-			const char *match;
 			struct ref *cpy = copy_ref(ref);
-			match = ref->name + remote_prefix_len;
-
-			cpy->peer_ref = alloc_ref(local_prefix_len +
-						  strlen(match) + 1);
-			sprintf(cpy->peer_ref->name, "%s%s",
-				refspec->dst, match);
-			if (refspec->force)
-				cpy->peer_ref->force = 1;
+
+			if (refspec->dst) {
+				const char *match = ref->name + 
+					remote_prefix_len;
+				cpy->peer_ref = alloc_ref(local_prefix_len +
+							  strlen(match) + 1);
+				sprintf(cpy->peer_ref->name, "%s%s",
+					refspec->dst, match);
+				if (refspec->force)
+					cpy->peer_ref->force = 1;
+			}
 			*tail = cpy;
 			tail = &cpy->next;
 		}

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help