threads / discuss / 8492
Subject: git-send-pack: broken handling of ref specs with wildcards
Alex RiesenTry 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.
Johannes SchindelinRe: git-send-pack: broken handling of ref specs with wildcards
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
Junio C HamanoRe: git-send-pack: broken handling of ref specs with wildcards
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.
Daniel BarkalowRe: git-send-pack: broken handling of ref specs with wildcards
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*
Alex Riesen[PATCH] Fix push with refspecs containing wildcards
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
Junio C HamanoRe: [PATCH] Fix push with refspecs containing wildcards
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.
Junio C HamanoRe: [PATCH] Fix push with refspecs containing wildcards
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
Alex RiesenRe: [PATCH] Fix push with refspecs containing wildcards
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>