git-send-pack: broken handling of ref specs with wildcards

git-send-pack: broken handling of ref specs with wildcards

[Alex Riesen]

Try something like this:

    git-send-pack --remote=origin --thin /some/other/repo \
    'refs/heads/*:refs/remotes/child/*'

The result looks broken: the sent reference are created not in
refs/remotes/child/ but just in refs/heads/ of /some/other/repo.

Re: git-send-pack: broken handling of ref specs with wildcards

[Johannes Schindelin]

Hi,

On Fri, 8 Jun 2007, Alex Riesen wrote:

> Try something like this:
> 
>     git-send-pack --remote=origin --thin /some/other/repo \
>     'refs/heads/*:refs/remotes/child/*'
> 
> The result looks broken: the sent reference are created not in
> refs/remotes/child/ but just in refs/heads/ of /some/other/repo.

I had the impression that it was git-push, a porcelain, which handles 
refspec wildcards, not send-pack, which is plumbing.

Ciao,
Dscho

Re: git-send-pack: broken handling of ref specs with wildcards

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Fri, 8 Jun 2007, Alex Riesen wrote:
>
>> Try something like this:
>> 
>>     git-send-pack --remote=origin --thin /some/other/repo \
>>     'refs/heads/*:refs/remotes/child/*'
>> 
>> The result looks broken: the sent reference are created not in
>> refs/remotes/child/ but just in refs/heads/ of /some/other/repo.
>
> I had the impression that it was git-push, a porcelain, which handles 
> refspec wildcards, not send-pack, which is plumbing.

Well, I do not think it is wrong per-se if plumbing send-pack
starts understanding 'a/*:b/*'.  Earlier it only understood
concrete refspecs, and such a refspec that has asterisks cannot
be concrete, as '*' is an invalid char in a refname.  So there
is no risk of confusion.

That is, as long as it is done right, which was not true in this
case.

Re: git-send-pack: broken handling of ref specs with wildcards

[Daniel Barkalow]

On Thu, 7 Jun 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Fri, 8 Jun 2007, Alex Riesen wrote:
> >
> >> Try something like this:
> >> 
> >>     git-send-pack --remote=origin --thin /some/other/repo \
> >>     'refs/heads/*:refs/remotes/child/*'
> >> 
> >> The result looks broken: the sent reference are created not in
> >> refs/remotes/child/ but just in refs/heads/ of /some/other/repo.
> >
> > I had the impression that it was git-push, a porcelain, which handles 
> > refspec wildcards, not send-pack, which is plumbing.
> 
> Well, I do not think it is wrong per-se if plumbing send-pack
> starts understanding 'a/*:b/*'.  Earlier it only understood
> concrete refspecs, and such a refspec that has asterisks cannot
> be concrete, as '*' is an invalid char in a refname.  So there
> is no risk of confusion.
> 
> That is, as long as it is done right, which was not true in this
> case.

Good catch; all of my configurations have a {foo}/*:{foo}/* pattern for 
the push, and I hadn't figured out a way to write tests for push and 
fetch.

The code looks like it should do the right thing... now that somebody's 
fixed it before I got to it. You people are far too efficient.

I'll probably break out a function for applying a pattern, since there are 
multiple places that need to do it, but that can go into a series I'm 
working on now.

	-Daniel
*This .sig left intentionally blank*

[PATCH] Fix push with refspecs containing wildcards

[Alex Riesen]

Otherwise

    git push 'remote-name' 'refs/heads/*:refs/remotes/other/*'

will consider references in "refs/heads" of the remote repository
"remote-name", instead of the ones in "refs/remotes/other", which
the given refspec clearly means.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
Alex Riesen, Fri, Jun 08, 2007 00:53:02 +0200:
> Try something like this:
> 
>     git-send-pack --remote=origin --thin /some/other/repo \
>     'refs/heads/*:refs/remotes/child/*'
> 
> The result looks broken: the sent reference are created not in
> refs/remotes/child/ but just in refs/heads/ of /some/other/repo.
> 

And as usual, I'm not sure if it is the right fix. Johannes has a
point: why the hell does send-pack parse the wildcards at all?!

 remote.c |   39 +++++++++++++++++++++++++++------------
 1 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/remote.c b/remote.c
index d904616..33c8e50 100644
--- a/remote.c
+++ b/remote.c
@@ -501,16 +501,16 @@ static struct ref *find_ref_by_name(struct ref *list, const char *name)
 	return NULL;
 }
 
-static int check_pattern_match(struct refspec *rs, int rs_nr, struct ref *src)
+static const struct refspec *check_pattern_match(const struct refspec *rs,
+						 int rs_nr,
+						 const struct ref *src)
 {
 	int i;
-	if (!rs_nr)
-		return 1;
 	for (i = 0; i < rs_nr; i++) {
 		if (rs[i].pattern && !prefixcmp(src->name, rs[i].src))
-			return 1;
+			return rs + i;
 	}
-	return 0;
+	return NULL;
 }
 
 int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,
@@ -525,29 +525,44 @@ int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,
 	/* pick the remainder */
 	for ( ; src; src = src->next) {
 		struct ref *dst_peer;
+		const struct refspec *pat = NULL;
+		char *dst_name;
 		if (src->peer_ref)
 			continue;
-		if (!check_pattern_match(rs, nr_refspec, src))
-			continue;
+		if (nr_refspec) {
+			pat = check_pattern_match(rs, nr_refspec, src);
+			if (!pat)
+				continue;
+		}
 
-		dst_peer = find_ref_by_name(dst, src->name);
+		if (pat) {
+			dst_name = xmalloc(strlen(pat->dst) +
+					   strlen(src->name) -
+					   strlen(pat->src) + 2);
+			strcpy(dst_name, pat->dst);
+			strcat(dst_name, src->name + strlen(pat->src));
+		} else
+			dst_name = strdup(src->name);
+		dst_peer = find_ref_by_name(dst, dst_name);
 		if (dst_peer && dst_peer->peer_ref)
 			/* We're already sending something to this ref. */
-			continue;
+			goto free_name;
 		if (!dst_peer && !nr_refspec && !all)
 			/* Remote doesn't have it, and we have no
 			 * explicit pattern, and we don't have
 			 * --all. */
-			continue;
+			goto free_name;
 		if (!dst_peer) {
 			/* Create a new one and link it */
-			int len = strlen(src->name) + 1;
+			int len = strlen(dst_name) + 1;
 			dst_peer = xcalloc(1, sizeof(*dst_peer) + len);
-			memcpy(dst_peer->name, src->name, len);
+			memcpy(dst_peer->name, dst_name, len);
 			hashcpy(dst_peer->new_sha1, src->new_sha1);
 			link_dst_tail(dst_peer, dst_tail);
 		}
 		dst_peer->peer_ref = src;
+	free_name:
+		free(dst_name);
 	}
 	return 0;
 }
-- 
1.5.2.1.147.g7c82d

Re: [PATCH] Fix push with refspecs containing wildcards

[Junio C Hamano]

Sending a fix is a good thing, but whenever doing one, could
people please also do a testcase that demonstrates the original
bug, and also a demonstration that the fix does not introduce
regression?

For this one, obviously a test for push that uses such wildcard
ref is needed but at the same time we would want a test for push
that does _not_ use a wildcard, fetch that uses a wildcard, and
a fetch that does not use a wildcard.

Re: [PATCH] Fix push with refspecs containing wildcards

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Sending a fix is a good thing, but whenever doing one, could
> people please also do a testcase that demonstrates the original
> bug, and also a demonstration that the fix does not introduce
> regression?
>
> For this one, obviously a test for push that uses such wildcard
> ref is needed but at the same time we would want a test for push
> that does _not_ use a wildcard, fetch that uses a wildcard, and
> a fetch that does not use a wildcard.

How about this?
---

 t/t5516-fetch-push.sh |   82 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 82 insertions(+), 0 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
new file mode 100755
index 0000000..dba018f
--- /dev/null
+++ b/t/t5516-fetch-push.sh
@@ -0,0 +1,80 @@
+#!/bin/sh
+
+test_description='fetching and pushing, with or without wildcard'
+
+. ./test-lib.sh
+
+mk_empty () {
+	rm -fr testrepo &&
+	mkdir testrepo &&
+	(
+		cd testrepo &&
+		git init
+	)
+}
+
+test_expect_success setup '
+
+	: >path1 &&
+	git add path1 &&
+	test_tick &&
+	git commit -a -m repo &&
+	the_commit=$(git show-ref -s --verify refs/heads/master)
+
+'
+
+test_expect_success 'fetch without wildcard' '
+	mk_empty &&
+	(
+		cd testrepo &&
+		git fetch .. refs/heads/master:refs/remotes/origin/master &&
+
+		r=$(git show-ref -s --verify refs/remotes/origin/master) &&
+		test "z$r" = "z$the_commit" &&
+
+		test 1 = $(git for-each-ref refs/remotes/origin | wc -l)
+	)
+'
+
+test_expect_success 'fetch with wildcard' '
+	mk_empty &&
+	(
+		cd testrepo &&
+		git config remote.up.url .. &&
+		git config remote.up.fetch "refs/heads/*:refs/remotes/origin/*" &&
+		git fetch up &&
+
+		r=$(git show-ref -s --verify refs/remotes/origin/master) &&
+		test "z$r" = "z$the_commit" &&
+
+		test 1 = $(git for-each-ref refs/remotes/origin | wc -l)
+	)
+'
+
+test_expect_success 'push without wildcard' '
+	mk_empty &&
+
+	git push testrepo refs/heads/master:refs/remotes/origin/master &&
+	(
+		cd testrepo &&
+		r=$(git show-ref -s --verify refs/remotes/origin/master) &&
+		test "z$r" = "z$the_commit" &&
+
+		test 1 = $(git for-each-ref refs/remotes/origin | wc -l)
+	)
+'
+
+test_expect_success 'push with wildcard' '
+	mk_empty &&
+
+	git push testrepo "refs/heads/*:refs/remotes/origin/*" &&
+	(
+		cd testrepo &&
+		r=$(git show-ref -s --verify refs/remotes/origin/master) &&
+		test "z$r" = "z$the_commit" &&
+
+		test 1 = $(git for-each-ref refs/remotes/origin | wc -l)
+	)
+'
+
+test_done

Re: [PATCH] Fix push with refspecs containing wildcards

[Alex Riesen]

Junio C Hamano, Fri, Jun 08, 2007 09:42:47 +0200:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Sending a fix is a good thing, but whenever doing one, could
> > people please also do a testcase that demonstrates the original
> > bug, and also a demonstration that the fix does not introduce
> > regression?

Sorry. I actually have a test script I used to develop the fix,
but somehow missed to send it.

> > For this one, obviously a test for push that uses such wildcard
> > ref is needed but at the same time we would want a test for push
> > that does _not_ use a wildcard, fetch that uses a wildcard, and
> > a fetch that does not use a wildcard.
> 
> How about this?
> ---
> 
>  t/t5516-fetch-push.sh |   82 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 82 insertions(+), 0 deletions(-)
> 

Gratefully-Acked-by: Alex Riesen <raa.lkml@gmail.com>