# Re: git-push segfault 4 messages from 2010-02-24 to 2010-02-24. Participants: ddrowley3@juno.com, Jeff King, Daniel Barkalow, Junio C Hamano. Thread: https://gitlist.dev/t/22795 ## ddrowley3@juno.com, 2010-02-24 16:27 Subject: Re: git-push segfault Message-ID: <20100224.082728.16634.0@webmail17.dca.untd.com> URL: https://gitlist.dev/e/20100224.082728.16634.0%40webmail17.dca.untd.com ``` > Does that trigger the problem for you? If not, can you show what is > different between your earlier reproduction recipe and this one? Yes, same problem. Here's the output: Initialized empty Git repository in /tmp/parent/.git/ Initialized empty Git repository in /tmp/child/.git/ warning: You appear to have cloned an empty repository. ./test.sh: line 12: 3686 Segmentation fault (core dumped) git push I reproduced this consistently on 2 different machines with 2 different versions of git, so it must have something to do with my ~/.gitconfig. Yep - if I remove the following from my .gitconfig, then the seg fault goes away: [push] default = tracking Dale ____________________________________________________________ Criminal Lawyer Criminal Lawyers - Click here. http://thirdpartyoffers.juno.com/TGL2131/c?cp=NyvAFq3GMltQMHm1-EJV8wAAJz1C6gppc1sGnWt7Fqwc41TnAAYAAAAAAAAAAAAAAAAAAADNAAAAAAAAAAAAAAAAAAAiFgAAAAA= ``` ## Jeff King, 2010-02-24 17:08 Subject: Re: git-push segfault Message-ID: <20100224170853.GA4756@coredump.intra.peff.net> URL: https://gitlist.dev/e/20100224170853.GA4756%40coredump.intra.peff.net In-Reply-To: <20100224.082728.16634.0@webmail17.dca.untd.com> ``` On Wed, Feb 24, 2010 at 04:27:28PM +0000, ddrowley3@juno.com wrote: > ./test.sh: line 12: 3686 Segmentation fault (core dumped) git > push > > I reproduced this consistently on 2 different machines with 2 > different versions of git, so it must have something to do with my > ~/.gitconfig. Yep - if I remove the following from my .gitconfig, then > the seg fault goes away: > > [push] > default = tracking Thanks, I can see it now. The patch below should fix it. Note, however, that you will still get a failure for "git push remote", as your config is set up to push tracking branches by default, and you don't have one here. I'm a little unsure of the patch. Arguably branch_get should not be setting branch->merge_nr to 1, as there is nothing in branch->merge. On the other hand, branch->merge_name _does_ have one element, so perhaps it is an error in the caller to assume that branch->merge_nr and branch->merge necessarily correspond. Daniel, this looks like your code. Comments? -- >8 -- Subject: [PATCH] push: fix segfault for odd config If you have a branch.$X.merge config option, but no branch.$X.remote, and your configuration tries to push tracking branches, git will segfault. The problem is that even though branch->merge_nr is 1, you don't actually have an upstream since there is no remote. Other callsites generally check explicitly that branch->merge is not NULL, so let's do that here, too. Signed-off-by: Jeff King --- builtin-push.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/builtin-push.c b/builtin-push.c index 5633f0a..f7bc2b2 100644 --- a/builtin-push.c +++ b/builtin-push.c @@ -68,7 +68,7 @@ static void setup_push_tracking(void) struct branch *branch = branch_get(NULL); if (!branch) die("You are not currently on a branch."); - if (!branch->merge_nr) + if (!branch->merge_nr || !branch->merge) die("The current branch %s is not tracking anything.", branch->name); if (branch->merge_nr != 1) -- 1.7.0.215.g2da3b.dirty ``` ## Daniel Barkalow, 2010-02-24 18:05 Subject: Re: git-push segfault Message-ID: URL: https://gitlist.dev/e/alpine.LNX.2.00.1002241218390.14365%40iabervon.org In-Reply-To: <20100224170853.GA4756@coredump.intra.peff.net> ``` On Wed, 24 Feb 2010, Jeff King wrote: > On Wed, Feb 24, 2010 at 04:27:28PM +0000, ddrowley3@juno.com wrote: > > > ./test.sh: line 12: 3686 Segmentation fault (core dumped) git > > push > > > > I reproduced this consistently on 2 different machines with 2 > > different versions of git, so it must have something to do with my > > ~/.gitconfig. Yep - if I remove the following from my .gitconfig, then > > the seg fault goes away: > > > > [push] > > default = tracking > > Thanks, I can see it now. The patch below should fix it. > > Note, however, that you will still get a failure for "git push remote", > as your config is set up to push tracking branches by default, and you > don't have one here. > > I'm a little unsure of the patch. Arguably branch_get should not be > setting branch->merge_nr to 1, as there is nothing in branch->merge. On > the other hand, branch->merge_name _does_ have one element, so perhaps > it is an error in the caller to assume that branch->merge_nr and > branch->merge necessarily correspond. Daniel, this looks like your code. > Comments? I think it would be worthwhile to have branch->merge, branch->merge_nr, and branch->merge_name all match, even if branch->remote is NULL, by discarding the merge_name if the remote isn't specified. It might also be wise to check branch_has_merge_config() instead of the test below, in any case, to match how the other code paths determine whether there is tracking configured. > -- >8 -- > Subject: [PATCH] push: fix segfault for odd config > > If you have a branch.$X.merge config option, but no > branch.$X.remote, and your configuration tries to push > tracking branches, git will segfault. > > The problem is that even though branch->merge_nr is 1, you > don't actually have an upstream since there is no remote. > Other callsites generally check explicitly that > branch->merge is not NULL, so let's do that here, too. > > Signed-off-by: Jeff King > --- > builtin-push.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/builtin-push.c b/builtin-push.c > index 5633f0a..f7bc2b2 100644 > --- a/builtin-push.c > +++ b/builtin-push.c > @@ -68,7 +68,7 @@ static void setup_push_tracking(void) > struct branch *branch = branch_get(NULL); > if (!branch) > die("You are not currently on a branch."); > - if (!branch->merge_nr) > + if (!branch->merge_nr || !branch->merge) > die("The current branch %s is not tracking anything.", > branch->name); > if (branch->merge_nr != 1) > -- > 1.7.0.215.g2da3b.dirty > > ``` ## Junio C Hamano, 2010-02-24 19:26 Subject: Re: git-push segfault Message-ID: <7vljeio2p9.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vljeio2p9.fsf%40alter.siamese.dyndns.org In-Reply-To: <20100224170853.GA4756@coredump.intra.peff.net> ``` Jeff King writes: > ... On > the other hand, branch->merge_name _does_ have one element, so perhaps > it is an error in the caller to assume that branch->merge_nr and > branch->merge necessarily correspond. I think this is a sensible thing to do. branch_has_merge_config() could be used here but I do not see a point, as the code already knows branch is non NULL and there are many other places that checks !!branch->merge themselves. It may be worth adding a helper function that a caller can lazily sanity check the set of configuration around a given branch, but that sanity check will probably go way beyond what branch_has_merge_config() currently does, so I would think that would be a separate patch after somebody audits what current non-users of branch_has_merge_config() want. For example, builtin-branch.c not only makes sure branch->merge is non NULL but also wants branch->merge[0] and branch->merge[0]->dst exists, and encapsulating only the first two out of four checks it does by using branch_has_merge_config() does not add much value to it.. Thanks. ```