git-send-pack SIGSEGV..

git-send-pack SIGSEGV..

[Linus Torvalds]

I haevn't had time to debug this at all, but

	git push --tags all

actually SIGSEGV's for me on the git repository. "all" is just a shorthand 
for the two repos I push my git repo to (so that I have it on some other 
machines too)

	[remote "all"]
		url = master.kernel.org:git
		url = login.linux-foundation.org:git.git

Without any real debugging at all, here's what I get:

	error: failed to push to 'master.kernel.org:git'
	error: send-pack died with strange error
	fatal: The remote end hung up unexpectedly
	error: failed to push to 'login.linux-foundation.org:git.git'
	error: send-pack died with strange error
	fatal: The remote end hung up unexpectedly

and a few core-files. The core-files look like so:

	Core was generated by `/home/torvalds/bin/git-send-pack --remote=all --thin master.kernel.org git refs'.
	Program terminated with signal 11, Segmentation fault.
	#0  0x0000003959477180 in strlen () from /lib64/libc.so.6
	(gdb) where
	#0  0x0000003959477180 in strlen () from /lib64/libc.so.6
	#1  0x000000000041cebe in match_refs (src=0x64b540, dst=<value optimized out>, dst_tail=0x62c890, nr_refspec=1, refspec=<value optimized out>, all=0) at remote.c:539
	#2  0x0000000000402e46 in main (argc=<value optimized out>, argv=0x7fffab2657a8) at send-pack.c:202

but I'm about to put the kids to bed, and I doubt I'll get to debug this 
before tomorrow. So I thought I'd send this out to Junio, Daniel and the 
list, in case somebody else gets to it first.

I *suspect* it's due to the refspec pattern matching changes Daniel did, 
but again - I haven't actually debugged it any deeper.

			Linus

Re: git-send-pack SIGSEGV..

[Linus Torvalds]

On Thu, 14 Jun 2007, Linus Torvalds wrote:

> 
> I haevn't had time to debug this at all, but
> 
> 	git push --tags all
> 
> actually SIGSEGV's for me on the git repository.

Oh, and this is with current 'master', of course. Commit 38570a47fc to be 
exact.

		Linus

Re: git-send-pack SIGSEGV..

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> I *suspect* it's due to the refspec pattern matching changes Daniel did, 
> but again - I haven't actually debugged it any deeper.

I am officially recuperating from an operation I had today, so I
cannot really take a deep look at this.

I think what is going wrong is that struct refspec for pattern
match that is parsed by parse_ref_spec does not have ->dst
component filled for "refs/tags/*" refspec, but match_refs()
does not check if pat->dst is NULL, in which case it should
reuse pat->src value.

Incidentally I have other remote.c fixes queued in 'next'.  I
haven't yet checked if I (accidentally) fixed this already.

Re: git-send-pack SIGSEGV..

[Junio C Hamano]

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

> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
>> I *suspect* it's due to the refspec pattern matching changes Daniel did, 
>> but again - I haven't actually debugged it any deeper.
>
> I am officially recuperating from an operation I had today, so I
> cannot really take a deep look at this.
>
> I think what is going wrong is that struct refspec for pattern
> match that is parsed by parse_ref_spec does not have ->dst
> component filled for "refs/tags/*" refspec, but match_refs()
> does not check if pat->dst is NULL, in which case it should
> reuse pat->src value.
>
> Incidentally I have other remote.c fixes queued in 'next'.  I
> haven't yet checked if I (accidentally) fixed this already.

Completely untested, but this may fix it.

I suspect this has an side effect of allowing

	fetch = refs/heads/*

to mean the same thing as

	fetch = refs/heads/*:refs/heads/*

which is suitable for a bare mirroring repository, but I do not
think of any downside, so it might be Ok.

But that is something from a person who was under anesthesia a
few hours ago, so you should take it with a big grain of salt ;-)

---

 remote.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/remote.c b/remote.c
index ed62a62..356e6bc 100644
--- a/remote.c
+++ b/remote.c
@@ -252,6 +252,8 @@ static struct refspec *parse_ref_spec(int nr_refspec, const char **refspec)
 			ep = gp;
 		}
 		rs[i].src = xstrndup(sp, ep - sp);
+		if (rs[i].pattern && !rs[i].dst)
+			rs[i].dst = xstrdup(rs[i].src);
 	}
 	return rs;
 }

Re: git-send-pack SIGSEGV..

[Alex Riesen]

On 6/15/07, Junio C Hamano <gitster@pobox.com> wrote:
> --- a/remote.c
> +++ b/remote.c
> @@ -252,6 +252,8 @@ static struct refspec *parse_ref_spec
>                         ep = gp;
>                 }
>                 rs[i].src = xstrndup(sp, ep - sp);
> +               if (rs[i].pattern && !rs[i].dst)
> +                       rs[i].dst = xstrdup(rs[i].src);

It may be possible to just reuse rs[i].src - there is no deallocation
of refspecs anywhere nor are src or dst detached from refspec
(which may be bad, but probably is covered in libification effort).

Re: git-send-pack SIGSEGV..

[Daniel Barkalow]

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

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Linus Torvalds <torvalds@linux-foundation.org> writes:
> >
> >> I *suspect* it's due to the refspec pattern matching changes Daniel did, 
> >> but again - I haven't actually debugged it any deeper.
> >
> > I am officially recuperating from an operation I had today, so I
> > cannot really take a deep look at this.
> >
> > I think what is going wrong is that struct refspec for pattern
> > match that is parsed by parse_ref_spec does not have ->dst
> > component filled for "refs/tags/*" refspec, but match_refs()
> > does not check if pat->dst is NULL, in which case it should
> > reuse pat->src value.
> >
> > Incidentally I have other remote.c fixes queued in 'next'.  I
> > haven't yet checked if I (accidentally) fixed this already.
> 
> Completely untested, but this may fix it.
> 
> I suspect this has an side effect of allowing
> 
> 	fetch = refs/heads/*
> 
> to mean the same thing as
> 
> 	fetch = refs/heads/*:refs/heads/*
> 
> which is suitable for a bare mirroring repository, but I do not
> think of any downside, so it might be Ok.
> 
> But that is something from a person who was under anesthesia a
> few hours ago, so you should take it with a big grain of salt ;-)

Yeah, that's not right; "push = refs/heads/*" works like that, but 
"fetch = refs/heads/*" puts them in MERGE_HEAD without storing them 
anywhere, unlike "fetch = refs/heads/*:refs/heads/*". That's why I didn't 
just copy the left side to the right side, which is what the old parsing 
code did: what you're going to do with the refspec determines how you 
interpret a missing rhs. So it needs to be the code that uses the refspec 
that handles this case.

Your analysis of the failure was right, though. I reproduced it, and this 
fixes it for me:

--- cut here ---
Author: Daniel Barkalow <barkalow@iabervon.org>
Date:   Fri Jun 15 10:22:37 2007 -0400

    Fix pushing to a pattern with no dst
    
    Refspecs with no colons are left with no dst value, because they are
    interepreted differently for fetch and push. For push, they mean to
    reuse the src side. Fix this for patterns.
    
    Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>

diff --git a/remote.c b/remote.c
index 6121416..c860740 100644
--- a/remote.c
+++ b/remote.c
@@ -546,10 +546,11 @@ int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,
 		}
 
 		if (pat) {
-			dst_name = xmalloc(strlen(pat->dst) +
+			const char *dst_side = pat->dst ? pat->dst : pat->src;
+ 			dst_name = xmalloc(strlen(dst_side) +
 					   strlen(src->name) -
 					   strlen(pat->src) + 2);
-			strcpy(dst_name, pat->dst);
+			strcpy(dst_name, dst_side);
 			strcat(dst_name, src->name + strlen(pat->src));
 		} else
 			dst_name = xstrdup(src->name);

Re: git-send-pack SIGSEGV..

[Linus Torvalds]

On Fri, 15 Jun 2007, Daniel Barkalow wrote:
> 
> Your analysis of the failure was right, though. I reproduced it, and this 
> fixes it for me:
> 
> --- cut here ---
> Author: Daniel Barkalow <barkalow@iabervon.org>
> Date:   Fri Jun 15 10:22:37 2007 -0400
> 
>     Fix pushing to a pattern with no dst
>     
>     Refspecs with no colons are left with no dst value, because they are
>     interepreted differently for fetch and push. For push, they mean to
>     reuse the src side. Fix this for patterns.
>     
>     Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>

And in case anybody cares, I've also verified it, so here's an

	Acked-by: Linus Torvalds <torvalds@linux-foundation.org>

in case Junio didn't already apply it.

		Linus

Re: git-send-pack SIGSEGV..

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

>> I suspect this has an side effect of allowing
>> 
>> 	fetch = refs/heads/*
>> 
>> to mean the same thing as
>> 
>> 	fetch = refs/heads/*:refs/heads/*
>> 
>> which is suitable for a bare mirroring repository, but I do not
>> think of any downside, so it might be Ok.
>> 
>> But that is something from a person who was under anesthesia a
>> few hours ago, so you should take it with a big grain of salt ;-)
>
> Yeah, that's not right; "push = refs/heads/*" works like that, but 
> "fetch = refs/heads/*" puts them in MERGE_HEAD without storing them 
> anywhere, unlike "fetch = refs/heads/*:refs/heads/*".

While I appreciate the proper fix, I have to say "Wait a minute".

First of all, do you really mean MERGE_HEAD in the above?  My
fix would obviously change the way how "git fetch" would STORE
what was fetched when you have "fetch = refs/heads/*" line in
the configuration file, which is what I already said in my
message.  However, using or not using tracking branches to store
the result should never affect if the fetched object is used in
MERGE_HEAD, so the fix you are responding to should not affect
what goes there at all.  If it does, then there is something
wrong in the current code.

I did not even realize that "push/fetch = $prefix/*" was
allowed.  IIRC, it has always been "$prefix1/*:$prefix2/*" form,
and "$prefix/*" was illegal.  Somehow a change in sematics
sneaked in without me knowing that makes it legal, and the
updated semantics is that for push "$prefix/*" is the same as
listing every ref under the prefix (without any colon, so it
pushes to the same name), and for fetch "$prefix/*" is the same
as $prefix/$x: (i.e. with colon and empty RHS) for all refs
under $prefix.

Now, I am not opposed to _having_ enhanced semantics.  Earlier,
"$prefix/*" was illegal, so it is not like you have introduced
any incompatibility.  On the push side, I do not think of any
sane interpretation for it other than treating it exactly as
"$prefix/*:$prefix/*".  On the fetch side, however, I am not
sure not to use any tracking _and_ grabbing everything is a sane
semantics.  You fetch everything under $prefix/ and list them in
FETCH_HEAD --- now what?  On the other handl, treating it as if
the user wrote "$prefix/*:$prefix/*" makes some sense.  It is a
natural operation to keep a bare mirroring repository up to
date.

But I am somewhat upset about us having the enhanced semantics
without me knowing.  I see two possible reasons for it: (1) I
was not being extra careful when I accepted your patches, and
(2) the change in the semantics were not documented, either in
the log nor Documentation/.

We obviously cannot fix (1) overnight, but could you please at
least fix (2)?  git-push.txt and pull-fetch-param.txt would be
the two places that talk about "$prefix1/*:$prefix2/*", and they
should talk about the new "$prefix/*" syntax as well.

I think "fetch = refs/heads/*" syntax, if it just fetches
without storing, does not make much sense.  In a separate-remote
repository, "[remote "foo"] fetch = refs/heads/*" would probably
be useful if we treated it as "refs/heads/*:refs/remotes/foo/*".
In a bare repository used for mirrors, it would be useful if it
stood for "refs/heads/*:refs/heads/*".

Re: git-send-pack SIGSEGV..

[Daniel Barkalow]

On Fri, 15 Jun 2007, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> >> I suspect this has an side effect of allowing
> >> 
> >> 	fetch = refs/heads/*
> >> 
> >> to mean the same thing as
> >> 
> >> 	fetch = refs/heads/*:refs/heads/*
> >> 
> >> which is suitable for a bare mirroring repository, but I do not
> >> think of any downside, so it might be Ok.
> >> 
> >> But that is something from a person who was under anesthesia a
> >> few hours ago, so you should take it with a big grain of salt ;-)
> >
> > Yeah, that's not right; "push = refs/heads/*" works like that, but 
> > "fetch = refs/heads/*" puts them in MERGE_HEAD without storing them 
> > anywhere, unlike "fetch = refs/heads/*:refs/heads/*".
> 
> While I appreciate the proper fix, I have to say "Wait a minute".
> 
> First of all, do you really mean MERGE_HEAD in the above? 

Sorry, FETCH_HEAD. MERGE_HEAD would be seriously wrong. I was trying to 
say that it didn't store them in any tracking location, and messed up the 
irrelevant bit of was it does do with them.

> I did not even realize that "push/fetch = $prefix/*" was
> allowed.  IIRC, it has always been "$prefix1/*:$prefix2/*" form,
> and "$prefix/*" was illegal. 

I thought when I was first writing the remotes.{c,h} code, I found that at 
least "push = foo/*" was supported already, due to nothing actually 
excluding it, and the code for copying the lhs to a blank rhs was being 
applied to patterns just like non-patterns.

That is, since "push = refs/heads/master" was already the same as "push = 
refs/heads/master:refs/heads/master", the pattern fell into that code. I 
was at least confidant enough of this to translate --tags into 
"refs/tags/*" instead of "refs/tags/*:refs/tags/*" when I got to that 
point.

I'm not sure if "fetch = foo/*" was permitted (or is, since fetch is still 
using other parsing in general), but, whatever it does, if it's allowed, 
it should certainly match "fetch = refs/head/master", which is pretty well 
agreed on at this point.

> Somehow a change in sematics sneaked in without me knowing that makes it 
> legal, and the updated semantics is that for push "$prefix/*" is the 
> same as listing every ref under the prefix (without any colon, so it
> pushes to the same name), and for fetch "$prefix/*" is the same
> as $prefix/$x: (i.e. with colon and empty RHS) for all refs
> under $prefix.
> 
> Now, I am not opposed to _having_ enhanced semantics.  Earlier,
> "$prefix/*" was illegal, so it is not like you have introduced
> any incompatibility.  On the push side, I do not think of any
> sane interpretation for it other than treating it exactly as
> "$prefix/*:$prefix/*".  On the fetch side, however, I am not
> sure not to use any tracking _and_ grabbing everything is a sane
> semantics.  You fetch everything under $prefix/ and list them in
> FETCH_HEAD --- now what?  On the other handl, treating it as if
> the user wrote "$prefix/*:$prefix/*" makes some sense.  It is a
> natural operation to keep a bare mirroring repository up to
> date.

I think it could make sense to prepare an octopus merge of a bunch of 
heads in a subdirectory. And it should either be prohibited or match the 
behavior of a non-pattern, or it would be really confusing.

> But I am somewhat upset about us having the enhanced semantics
> without me knowing.  I see two possible reasons for it: (1) I
> was not being extra careful when I accepted your patches, and
> (2) the change in the semantics were not documented, either in
> the log nor Documentation/.

Actually, I thought I was just overlooking the part of the documentation 
that explained patterns for push, but it isn't actually there. 
builtin-push.c put a lot of effort into supporting them when I got to it, 
but none of the documentation even suggests that you can put wildcards in 
two-sided refspecs for push, let alone one (and the code I was looking at 
was yours, added without documentation but with a hefty commit message in 
d46ae3f0). Based on the meaning of wildcards in push refspecs, however, 
the documentation for one-sided refspecs in git-push:

 A parameter <ref> without a colon pushes the <ref> from the source 
 repository to the destination repository under the same name.

clearly implies that anything that, whatever refs/heads/* matches, it 
pushes to the destination under the same name.

On the fetch side, the code isn't using my parser yet, anyway. But my 
parser should be able to distinguish the one-sided refspec case, so that 
the fetch logic can do whatever is determined to be right with the 
combination of features.

> I think "fetch = refs/heads/*" syntax, if it just fetches
> without storing, does not make much sense.  In a separate-remote
> repository, "[remote "foo"] fetch = refs/heads/*" would probably
> be useful if we treated it as "refs/heads/*:refs/remotes/foo/*".
> In a bare repository used for mirrors, it would be useful if it
> stood for "refs/heads/*:refs/heads/*".

I'm kind of uncomfortable with this level of complexity for a fully 
specified lhs pattern. Maybe "fetch = refs/heads/*" should be prohibited, 
while something DWIM-y like "fetch = heads/*" or even "fetch = head *" 
could come up with clever and useful patterns. In any case, the parser 
should report "refs/heads/*" as pattern,refs/heads/,NULL and let the fetch 
code decide what to do with it, rather than having the special case in the 
parser.

	-Daniel
*This .sig left intentionally blank*

Re: git-send-pack SIGSEGV..

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> That is, since "push = refs/heads/master" was already the same as "push = 
> refs/heads/master:refs/heads/master", the pattern fell into that code. I 
> was at least confidant enough of this to translate --tags into 
> "refs/tags/*" instead of "refs/tags/*:refs/tags/*" when I got to that 
> point.

This change sounds sensible.

> On the fetch side, the code isn't using my parser yet, anyway. But my 
> parser should be able to distinguish the one-sided refspec case, so that 
> the fetch logic can do whatever is determined to be right with the 
> combination of features.

Yes, but at the same time I know you are planning to rewrite
git-fetch using this code, so...

>> I think "fetch = refs/heads/*" syntax, if it just fetches
>> without storing, does not make much sense.  In a separate-remote
>> repository, "[remote "foo"] fetch = refs/heads/*" would probably
>> be useful if we treated it as "refs/heads/*:refs/remotes/foo/*".
>> In a bare repository used for mirrors, it would be useful if it
>> stood for "refs/heads/*:refs/heads/*".
>
> I'm kind of uncomfortable with this level of complexity for a fully 
> specified lhs pattern. Maybe "fetch = refs/heads/*" should be prohibited, 
> while something DWIM-y like "fetch = heads/*" or even "fetch = head *" 
> could come up with clever and useful patterns. In any case, the parser 
> should report "refs/heads/*" as pattern,refs/heads/,NULL and let the fetch 
> code decide what to do with it, rather than having the special case in the 
> parser.

Yeah, I am not very happy about DWIMmery either, especially a
magic that changes behaviour depending on the bareness [*1*].
As long as the parser can distinguish these five cases, I'd be
happy.

                        [push]			[fetch]
 (1) $any/*		same name		TBD
 (2) $any/*:		error			TBD
 (3) $any1/*:$any2/*	map any1 => any2	map any1 => any2
 (4) :$any/*		error			error
 (5) $any1/*:$any2	error			error
     $any1:$any2/*

(1-fetch) should probably be an error to avoid newbie confusion.

(2-push) is clearly an error.

(2-fetch) does not make much sense (octopus of random branches
    you do not even control what they are), but I do not have
    too strong an objection against it, so marked it TBD for now.

(3) is what we have always done.

(4-fetch) is clearly an error.

(4-push) could be "remove all refs", but I'd rather make that an
error.

(5) is clearly an error.  If you explicitly have both sides,
    they should both be wildcard, or non-wildcard.

[Footnote]

*1* Sometimes I see an incorrect suggestion given on #git to do
"mv .git ../project.git" to "make an incorrectly made repository
bare".  This is incorrect---at least you must update core.bare,
and also should make sure .git/object/info/alternates points at
a right directory if you used a relative path to specify one.
Luckily, we see much smaller number of such incorrect "help" on
the mailing list.