{"thread":{"id":"22795","subject":"Re: git-push segfault","startedAt":"2010-02-24T16:27:28Z","lastAt":"2010-02-24T19:26:10Z","messageCount":4,"participants":["ddrowley3@juno.com","Jeff King","Daniel Barkalow","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"135579","messageId":"20100224.082728.16634.0@webmail17.dca.untd.com","threadId":"22795","inReplyTo":null,"subject":"Re: git-push segfault","fromName":"ddrowley3@juno.com","fromEmail":"ddrowley3@juno.com","sentAt":"2010-02-24T16:27:28Z","receivedAt":"2010-02-24T16:27:28Z","isPatch":false,"sender":{"key":"ddrowley3@juno.com","avatar":null},"body":"\n> Does that trigger the problem for you? If not, can you show what is\n> different between your earlier reproduction recipe and this one?\n\nYes, same problem. Here's the output:\n\nInitialized empty Git repository in /tmp/parent/.git/\nInitialized empty Git repository in /tmp/child/.git/\nwarning: You appear to have cloned an empty repository.\n./test.sh: line 12:  3686 Segmentation fault      (core dumped) git push\n\nI 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:\n\n[push]\n    default = tracking\n\n\nDale\n\n\n____________________________________________________________\nCriminal Lawyer\nCriminal Lawyers - Click here.\nhttp://thirdpartyoffers.juno.com/TGL2131/c?cp=NyvAFq3GMltQMHm1-EJV8wAAJz1C6gppc1sGnWt7Fqwc41TnAAYAAAAAAAAAAAAAAAAAAADNAAAAAAAAAAAAAAAAAAAiFgAAAAA=\n"},{"id":"135581","messageId":"20100224170853.GA4756@coredump.intra.peff.net","threadId":"22795","inReplyTo":"20100224.082728.16634.0@webmail17.dca.untd.com","subject":"Re: git-push segfault","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-24T17:08:53Z","receivedAt":"2010-02-24T17:08:53Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 24, 2010 at 04:27:28PM +0000, ddrowley3@juno.com wrote:\n\n> ./test.sh: line 12:  3686 Segmentation fault      (core dumped) git\n> push\n> \n> I reproduced this consistently on 2 different machines with 2\n> different versions of git, so it must have something to do with my\n> ~/.gitconfig. Yep - if I remove the following from my .gitconfig, then\n> the seg fault goes away:\n> \n> [push]\n>     default = tracking\n\nThanks, I can see it now. The patch below should fix it.\n\nNote, however, that you will still get a failure for \"git push remote\",\nas your config is set up to push tracking branches by default, and you\ndon't have one here.\n\nI'm a little unsure of the patch. Arguably branch_get should not be\nsetting branch->merge_nr to 1, as there is nothing in branch->merge. On\nthe other hand, branch->merge_name _does_ have one element, so perhaps\nit is an error in the caller to assume that branch->merge_nr and\nbranch->merge necessarily correspond. Daniel, this looks like your code.\nComments?\n\n-- >8 --\nSubject: [PATCH] push: fix segfault for odd config\n\nIf you have a branch.$X.merge config option, but no\nbranch.$X.remote, and your configuration tries to push\ntracking branches, git will segfault.\n\nThe problem is that even though branch->merge_nr is 1, you\ndon't actually have an upstream since there is no remote.\nOther callsites generally check explicitly that\nbranch->merge is not NULL, so let's do that here, too.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin-push.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-push.c b/builtin-push.c\nindex 5633f0a..f7bc2b2 100644\n--- a/builtin-push.c\n+++ b/builtin-push.c\n@@ -68,7 +68,7 @@ static void setup_push_tracking(void)\n \tstruct branch *branch = branch_get(NULL);\n \tif (!branch)\n \t\tdie(\"You are not currently on a branch.\");\n-\tif (!branch->merge_nr)\n+\tif (!branch->merge_nr || !branch->merge)\n \t\tdie(\"The current branch %s is not tracking anything.\",\n \t\t    branch->name);\n \tif (branch->merge_nr != 1)\n-- \n1.7.0.215.g2da3b.dirty\n"},{"id":"135589","messageId":"alpine.LNX.2.00.1002241218390.14365@iabervon.org","threadId":"22795","inReplyTo":"20100224170853.GA4756@coredump.intra.peff.net","subject":"Re: git-push segfault","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2010-02-24T18:05:44Z","receivedAt":"2010-02-24T18:05:44Z","isPatch":false,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Wed, 24 Feb 2010, Jeff King wrote:\n\n> On Wed, Feb 24, 2010 at 04:27:28PM +0000, ddrowley3@juno.com wrote:\n> \n> > ./test.sh: line 12:  3686 Segmentation fault      (core dumped) git\n> > push\n> > \n> > I reproduced this consistently on 2 different machines with 2\n> > different versions of git, so it must have something to do with my\n> > ~/.gitconfig. Yep - if I remove the following from my .gitconfig, then\n> > the seg fault goes away:\n> > \n> > [push]\n> >     default = tracking\n> \n> Thanks, I can see it now. The patch below should fix it.\n> \n> Note, however, that you will still get a failure for \"git push remote\",\n> as your config is set up to push tracking branches by default, and you\n> don't have one here.\n> \n> I'm a little unsure of the patch. Arguably branch_get should not be\n> setting branch->merge_nr to 1, as there is nothing in branch->merge. On\n> the other hand, branch->merge_name _does_ have one element, so perhaps\n> it is an error in the caller to assume that branch->merge_nr and\n> branch->merge necessarily correspond. Daniel, this looks like your code.\n> Comments?\n\nI think it would be worthwhile to have branch->merge, branch->merge_nr, \nand branch->merge_name all match, even if branch->remote is NULL, by \ndiscarding the merge_name if the remote isn't specified.\n\nIt might also be wise to check branch_has_merge_config() instead of the \ntest below, in any case, to match how the other code paths determine \nwhether there is tracking configured.\n\n> -- >8 --\n> Subject: [PATCH] push: fix segfault for odd config\n> \n> If you have a branch.$X.merge config option, but no\n> branch.$X.remote, and your configuration tries to push\n> tracking branches, git will segfault.\n> \n> The problem is that even though branch->merge_nr is 1, you\n> don't actually have an upstream since there is no remote.\n> Other callsites generally check explicitly that\n> branch->merge is not NULL, so let's do that here, too.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  builtin-push.c |    2 +-\n>  1 files changed, 1 insertions(+), 1 deletions(-)\n> \n> diff --git a/builtin-push.c b/builtin-push.c\n> index 5633f0a..f7bc2b2 100644\n> --- a/builtin-push.c\n> +++ b/builtin-push.c\n> @@ -68,7 +68,7 @@ static void setup_push_tracking(void)\n>  \tstruct branch *branch = branch_get(NULL);\n>  \tif (!branch)\n>  \t\tdie(\"You are not currently on a branch.\");\n> -\tif (!branch->merge_nr)\n> +\tif (!branch->merge_nr || !branch->merge)\n>  \t\tdie(\"The current branch %s is not tracking anything.\",\n>  \t\t    branch->name);\n>  \tif (branch->merge_nr != 1)\n> -- \n> 1.7.0.215.g2da3b.dirty\n> \n> \n"},{"id":"135602","messageId":"7vljeio2p9.fsf@alter.siamese.dyndns.org","threadId":"22795","inReplyTo":"20100224170853.GA4756@coredump.intra.peff.net","subject":"Re: git-push segfault","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-24T19:26:10Z","receivedAt":"2010-02-24T19:26:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> ... On\n> the other hand, branch->merge_name _does_ have one element, so perhaps\n> it is an error in the caller to assume that branch->merge_nr and\n> branch->merge necessarily correspond.\n\nI think this is a sensible thing to do.   branch_has_merge_config() could\nbe used here but I do not see a point, as the code already knows branch is\nnon NULL and there are many other places that checks !!branch->merge\nthemselves.\n\nIt may be worth adding a helper function that a caller can lazily sanity\ncheck the set of configuration around a given branch, but that sanity\ncheck will probably go way beyond what branch_has_merge_config() currently\ndoes, so I would think that would be a separate patch after somebody\naudits what current non-users of branch_has_merge_config() want.  For\nexample, builtin-branch.c not only makes sure branch->merge is non NULL\nbut also wants branch->merge[0] and branch->merge[0]->dst exists, and\nencapsulating only the first two out of four checks it does by using\nbranch_has_merge_config() does not add much value to it..\n\nThanks.\n"}]}