{"thread":{"id":"27335","subject":"git cherry-pick --strategy=resolve segfaults if picking a root commit","startedAt":"2011-05-12T10:15:24Z","lastAt":"2011-05-16T15:01:33Z","messageCount":12,"participants":["Sebastian Schuberth","Jeff King","Johannes Sixt","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"167717","messageId":"iqgbvs$pm5$1@dough.gmane.org","threadId":"27335","inReplyTo":null,"subject":"git cherry-pick --strategy=resolve segfaults if picking a root commit","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2011-05-12T10:15:24Z","receivedAt":"2011-05-12T10:15:24Z","isPatch":false,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"Hi,\n\nI just noticed that (under both Linux and Windows) Git 1.7.5 segfaults \nfor me if cherry-picking a root commit from one repo into another one \nwith \"--strategy=resolve\". It does not segfault if \"--strategy=resolve\" \nis omitted. Here's what I did:\n\n# Create a repository with only a root commit.\nmkdir repo1\ncd repo1\ngit init\n\necho \"test1\" > test1\ngit add test1\ngit commit -m \"test1\"\n\n# Create another repository with only a root commit.\nmkdir ../repo2\ncd ../repo2\ngit init\n\necho \"test2\" > test2\ngit add test2\ngit commit -m \"test2\"\n\ngit remote add repo1 ../repo1\ngit fetch repo1\n\n# Cherry-pick with \"--strategy=resolve\" -> segfaults.\ngit cherry-pick --strategy=resolve repo1/master\n\nIs this already a known issue?\n\n-- \nSebastian Schuberth\n"},{"id":"167718","messageId":"20110512104558.GA22108@sigill.intra.peff.net","threadId":"27335","inReplyTo":"iqgbvs$pm5$1@dough.gmane.org","subject":"Re: git cherry-pick --strategy=resolve segfaults if picking a root commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-12T10:45:58Z","receivedAt":"2011-05-12T10:45:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 12, 2011 at 12:15:24PM +0200, Sebastian Schuberth wrote:\n\n> I just noticed that (under both Linux and Windows) Git 1.7.5\n> segfaults for me if cherry-picking a root commit from one repo into\n> another one with \"--strategy=resolve\". It does not segfault if\n> \"--strategy=resolve\" is omitted. Here's what I did:\n> [...]\n> Is this already a known issue?\n\nNo, I think you found a new one (I doubt many people use alternate\nstrategies for cherry-picking, and probably even less frequently on root\ncommits).\n\nHere's a fix, but see below.\n\n---\n builtin/revert.c |    3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex f697e66..2eebb58 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -497,7 +497,8 @@ static int do_pick_commit(void)\n \n \t\twrite_message(&msgbuf, defmsg);\n \n-\t\tcommit_list_insert(base, &common);\n+\t\tif (base)\n+\t\t\tcommit_list_insert(base, &common);\n \t\tcommit_list_insert(next, &remotes);\n \t\tres = try_merge_command(strategy, xopts_nr, xopts, common,\n \t\t\t\t\tsha1_to_hex(head), remotes);\n\nThe base commit comes from the \"parent\" pointer in a cherry, which is\ngoing to be NULL in this case. In a revert, however, the parent pointer\nends up in \"next\". However, you can't get a segfault with \"git revert\n--strategy=resolve $some_root_commit\", because earlier we explicitly\ndisallow reverting a root commit.\n\nSo this patch makes it stop segfaulting, but I'm not sure that it is\ndoing anything useful. Merge-resolve seems to just barf, whereas\nmerge-recursive properly handles this case (it cherry-picks the\ndifference between the empty tree and the root commit's tree).\n\nSo probably we should:\n\n  1. Pass the empty tree along to merge-resolve. This will take a little\n     bit of refactoring, but more importantly, it means we will be\n     passing a tree-ish and not a commit-ish to a merge strategy. Is\n     that OK?\n\n  2. Consider lifting the restriction on reverting root commits. If we\n     can cherry-pick it, we can revert it, so I suspect this would\n     already work with merge-recursive, but I didn't try. I don't care\n     too much either way, though; I doubt it's something people would do\n     a lot. It just seems like an unnecessary restriction.\n\n-Peff\n"},{"id":"167720","messageId":"20110512110855.GA5240@sigill.intra.peff.net","threadId":"27335","inReplyTo":"20110512104558.GA22108@sigill.intra.peff.net","subject":"Re: git cherry-pick --strategy=resolve segfaults if picking a root commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-12T11:08:55Z","receivedAt":"2011-05-12T11:08:55Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 12, 2011 at 06:45:58AM -0400, Jeff King wrote:\n\n> So probably we should:\n> \n>   1. Pass the empty tree along to merge-resolve. This will take a little\n>      bit of refactoring, but more importantly, it means we will be\n>      passing a tree-ish and not a commit-ish to a merge strategy. Is\n>      that OK?\n> \n>   2. Consider lifting the restriction on reverting root commits. If we\n>      can cherry-pick it, we can revert it, so I suspect this would\n>      already work with merge-recursive, but I didn't try. I don't care\n>      too much either way, though; I doubt it's something people would do\n>      a lot. It just seems like an unnecessary restriction.\n\nThis turned out to be quite easy. git-merge-resolve handles the tree-ish\nargument just fine. But it's possible other merge helpers might not be\nso happy. I dunno.\n\nThe series is:\n\n  [1/3]: cherry-pick: handle root commits with external strategies\n  [2/3]: revert: allow reverting a root commit\n  [3/3]: t3503: test cherry picking and reverting root commits\n\n-Peff\n"},{"id":"167721","messageId":"20110512110946.GA5292@sigill.intra.peff.net","threadId":"27335","inReplyTo":"20110512110855.GA5240@sigill.intra.peff.net","subject":"[PATCH 1/3] cherry-pick: handle root commits with external strategies","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-12T11:09:46Z","receivedAt":"2011-05-12T11:09:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The merge-recursive strategy already handles root commits;\nit cherry-picks the difference between the empty tree and\nthe root commit's tree.\n\nHowever, for external strategies, we dereference NULL and\nsegfault while building the argument list. Instead, let's\nhandle this by passing the empty tree sha1 to the merge\nscript.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/merge.c |   12 ++++++++++--\n 1 files changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 9661c8f..5098bf6 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -599,6 +599,14 @@ static void write_tree_trivial(unsigned char *sha1)\n \t\tdie(_(\"git write-tree failed to write a tree\"));\n }\n \n+static const char *merge_argument(struct commit *commit)\n+{\n+\tif (commit)\n+\t\treturn sha1_to_hex(commit->object.sha1);\n+\telse\n+\t\treturn EMPTY_TREE_SHA1_HEX;\n+}\n+\n int try_merge_command(const char *strategy, size_t xopts_nr,\n \t\t      const char **xopts, struct commit_list *common,\n \t\t      const char *head_arg, struct commit_list *remotes)\n@@ -619,11 +627,11 @@ int try_merge_command(const char *strategy, size_t xopts_nr,\n \t\targs[i++] = s;\n \t}\n \tfor (j = common; j; j = j->next)\n-\t\targs[i++] = xstrdup(sha1_to_hex(j->item->object.sha1));\n+\t\targs[i++] = xstrdup(merge_argument(j->item));\n \targs[i++] = \"--\";\n \targs[i++] = head_arg;\n \tfor (j = remotes; j; j = j->next)\n-\t\targs[i++] = xstrdup(sha1_to_hex(j->item->object.sha1));\n+\t\targs[i++] = xstrdup(merge_argument(j->item));\n \targs[i] = NULL;\n \tret = run_command_v_opt(args, RUN_GIT_CMD);\n \tstrbuf_release(&buf);\n-- \n1.7.5.1.12.ga7abed\n"},{"id":"167722","messageId":"20110512110955.GB5292@sigill.intra.peff.net","threadId":"27335","inReplyTo":"20110512110855.GA5240@sigill.intra.peff.net","subject":"[PATCH 2/3] revert: allow reverting a root commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-12T11:09:55Z","receivedAt":"2011-05-12T11:09:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Although it is probably an uncommon operation, there is no\nreason to disallow it, as it works just fine. It is the\nreverse of a cherry-pick of a root commit, which is already\nallowed.\n\nWe do have to tweak one check on whether we have a merge\ncommit, which assumed we had at least one parent.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/revert.c |    4 +---\n 1 files changed, 1 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex f697e66..1f27c63 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -408,8 +408,6 @@ static int do_pick_commit(void)\n \tdiscard_cache();\n \n \tif (!commit->parents) {\n-\t\tif (action == REVERT)\n-\t\t\tdie (_(\"Cannot revert a root commit\"));\n \t\tparent = NULL;\n \t}\n \telse if (commit->parents->next) {\n@@ -467,7 +465,7 @@ static int do_pick_commit(void)\n \t\tstrbuf_addstr(&msgbuf, \"\\\"\\n\\nThis reverts commit \");\n \t\tstrbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));\n \n-\t\tif (commit->parents->next) {\n+\t\tif (commit->parents && commit->parents->next) {\n \t\t\tstrbuf_addstr(&msgbuf, \", reversing\\nchanges made to \");\n \t\t\tstrbuf_addstr(&msgbuf, sha1_to_hex(parent->object.sha1));\n \t\t}\n-- \n1.7.5.1.12.ga7abed\n"},{"id":"167723","messageId":"20110512111007.GC5292@sigill.intra.peff.net","threadId":"27335","inReplyTo":"20110512110855.GA5240@sigill.intra.peff.net","subject":"[PATCH 3/3] t3503: test cherry picking and reverting root commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-12T11:10:07Z","receivedAt":"2011-05-12T11:10:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We already tested cherry-picking a root commit, but only\nwith the internal merge-recursive strategy. Let's also test\nthe recently-allowed reverting of a root commit, as well as\ntesting with external strategies (which until recently\ntriggered a segfault).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t3503-cherry-pick-root.sh |   23 ++++++++++++++++++++++-\n 1 files changed, 22 insertions(+), 1 deletions(-)\n\ndiff --git a/t/t3503-cherry-pick-root.sh b/t/t3503-cherry-pick-root.sh\nindex b0faa29..1f9ed67 100755\n--- a/t/t3503-cherry-pick-root.sh\n+++ b/t/t3503-cherry-pick-root.sh\n@@ -1,6 +1,6 @@\n #!/bin/sh\n \n-test_description='test cherry-picking a root commit'\n+test_description='test cherry-picking (and reverting) a root commit'\n \n . ./test-lib.sh\n \n@@ -27,4 +27,25 @@ test_expect_success 'cherry-pick a root commit' '\n \n '\n \n+test_expect_success 'revert a root commit' '\n+\n+\tgit revert master &&\n+\t! test -f file1\n+\n+'\n+\n+test_expect_success 'cherry-pick a root commit with an external strategy' '\n+\n+\tgit cherry-pick --strategy=resolve master &&\n+\ttest first = $(cat file1)\n+\n+'\n+\n+test_expect_success 'revert a root commit with an external strategy' '\n+\n+\tgit revert --strategy=resolve master &&\n+\t! test -f file1\n+\n+'\n+\n test_done\n-- \n1.7.5.1.12.ga7abed\n"},{"id":"167729","messageId":"4DCBD087.90302@viscovery.net","threadId":"27335","inReplyTo":"20110512111007.GC5292@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] t3503: test cherry picking and reverting root commits","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-05-12T12:20:23Z","receivedAt":"2011-05-12T12:20:23Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 5/12/2011 13:10, schrieb Jeff King:\n> +test_expect_success 'cherry-pick a root commit with an external strategy' '\n> +\n> +\tgit cherry-pick --strategy=resolve master &&\n> +\ttest first = $(cat file1)\n\nWhat if file1 does not exist? Then cat fails loudly. But this does not\nfail the entire command immediately; rather, the test command fails, but\nnot because of a non-equality, but because of an invalid usage (\"syntax\nerror\"). IOW, the test does the right thing, but for the wrong reason.\n\nYes, an earlier test gave a bad precedent, and the following fixup (to\nbe squashed in) fixes it, too.\n\n-- Hannes\n\n\ndiff --git a/t/t3503-cherry-pick-root.sh b/t/t3503-cherry-pick-root.sh\nindex 1f9ed67..3006452 100755\n--- a/t/t3503-cherry-pick-root.sh\n+++ b/t/t3503-cherry-pick-root.sh\n@@ -23,7 +23,7 @@ test_expect_success setup '\n test_expect_success 'cherry-pick a root commit' '\n \n \tgit cherry-pick master &&\n-\ttest first = $(cat file1)\n+\ttest first = \"$(cat file1)\"\n \n '\n \n@@ -37,7 +37,7 @@ test_expect_success 'revert a root commit' '\n test_expect_success 'cherry-pick a root commit with an external strategy' '\n \n \tgit cherry-pick --strategy=resolve master &&\n-\ttest first = $(cat file1)\n+\ttest first = \"$(cat file1)\"\n \n '\n \n"},{"id":"167730","messageId":"20110512123703.GA21078@sigill.intra.peff.net","threadId":"27335","inReplyTo":"4DCBD087.90302@viscovery.net","subject":"Re: [PATCH 3/3] t3503: test cherry picking and reverting root commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-12T12:37:03Z","receivedAt":"2011-05-12T12:37:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 12, 2011 at 02:20:23PM +0200, Johannes Sixt wrote:\n\n> Am 5/12/2011 13:10, schrieb Jeff King:\n> > +test_expect_success 'cherry-pick a root commit with an external strategy' '\n> > +\n> > +\tgit cherry-pick --strategy=resolve master &&\n> > +\ttest first = $(cat file1)\n> \n> What if file1 does not exist? Then cat fails loudly. But this does not\n> fail the entire command immediately; rather, the test command fails, but\n> not because of a non-equality, but because of an invalid usage (\"syntax\n> error\"). IOW, the test does the right thing, but for the wrong reason.\n> \n> Yes, an earlier test gave a bad precedent, and the following fixup (to\n> be squashed in) fixes it, too.\n\nYeah, sorry, I blindly copied the earlier test without thinking. If we\nare going to tweak, my preference is actually to use test_cmp. And while\nwe're at it, I should be using test_path_is_missing.\n\nSo:\n\ndiff --git a/t/t3503-cherry-pick-root.sh b/t/t3503-cherry-pick-root.sh\nindex 1f9ed67..9aefe3a 100755\n--- a/t/t3503-cherry-pick-root.sh\n+++ b/t/t3503-cherry-pick-root.sh\n@@ -23,28 +23,30 @@ test_expect_success setup '\n test_expect_success 'cherry-pick a root commit' '\n \n \tgit cherry-pick master &&\n-\ttest first = $(cat file1)\n+\techo first >expect &&\n+\ttest_cmp expect file1\n \n '\n \n test_expect_success 'revert a root commit' '\n \n \tgit revert master &&\n-\t! test -f file1\n+\ttest_path_is_missing file1\n \n '\n \n test_expect_success 'cherry-pick a root commit with an external strategy' '\n \n \tgit cherry-pick --strategy=resolve master &&\n-\ttest first = $(cat file1)\n+\techo first >expect &&\n+\ttest_cmp expect file1\n \n '\n \n test_expect_success 'revert a root commit with an external strategy' '\n \n \tgit revert --strategy=resolve master &&\n-\t! test -f file1\n+\ttest_path_is_missing file1\n \n '\n \n"},{"id":"167736","messageId":"BANLkTinyLeTL=-rUPEZOQapudWCR_Lbh4w@mail.gmail.com","threadId":"27335","inReplyTo":"20110512110946.GA5292@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] cherry-pick: handle root commits with external strategies","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2011-05-12T14:50:11Z","receivedAt":"2011-05-12T14:50:11Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Thu, May 12, 2011 at 13:09, Jeff King <peff@peff.net> wrote:\n\n> The merge-recursive strategy already handles root commits;\n> it cherry-picks the difference between the empty tree and\n> the root commit's tree.\n>\n> However, for external strategies, we dereference NULL and\n> segfault while building the argument list. Instead, let's\n> handle this by passing the empty tree sha1 to the merge\n> script.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n\nWorks perfectly for me, thanks a lot for coming up with a patch incredibly fast!\n\n-- \nSebastian Schuberth\n"},{"id":"167737","messageId":"BANLkTimMc5LRTWpy1Ka+Gf_Cu5SAN785TQ@mail.gmail.com","threadId":"27335","inReplyTo":"20110512110955.GB5292@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] revert: allow reverting a root commit","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2011-05-12T14:53:31Z","receivedAt":"2011-05-12T14:53:31Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Thu, May 12, 2011 at 13:09, Jeff King <peff@peff.net> wrote:\n\n> Although it is probably an uncommon operation, there is no\n> reason to disallow it, as it works just fine. It is the\n> reverse of a cherry-pick of a root commit, which is already\n> allowed.\n>\n> We do have to tweak one check on whether we have a merge\n> commit, which assumed we had at least one parent.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n\nI've quickly tested this one, too, and it also seems to work nicely.\nGreat stuff!\n\n-- \nSebastian Schuberth\n"},{"id":"167979","messageId":"20110516102744.GA23286@sigill.intra.peff.net","threadId":"27335","inReplyTo":"20110512110855.GA5240@sigill.intra.peff.net","subject":"Re: git cherry-pick --strategy=resolve segfaults if picking a root commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-16T10:27:44Z","receivedAt":"2011-05-16T10:27:44Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 12, 2011 at 07:08:55AM -0400, Jeff King wrote:\n\n> On Thu, May 12, 2011 at 06:45:58AM -0400, Jeff King wrote:\n> \n> > So probably we should:\n> > \n> >   1. Pass the empty tree along to merge-resolve. This will take a little\n> >      bit of refactoring, but more importantly, it means we will be\n> >      passing a tree-ish and not a commit-ish to a merge strategy. Is\n> >      that OK?\n> > \n> >   2. Consider lifting the restriction on reverting root commits. If we\n> >      can cherry-pick it, we can revert it, so I suspect this would\n> >      already work with merge-recursive, but I didn't try. I don't care\n> >      too much either way, though; I doubt it's something people would do\n> >      a lot. It just seems like an unnecessary restriction.\n> \n> This turned out to be quite easy. git-merge-resolve handles the tree-ish\n> argument just fine. But it's possible other merge helpers might not be\n> so happy. I dunno.\n> \n> The series is:\n> \n>   [1/3]: cherry-pick: handle root commits with external strategies\n>   [2/3]: revert: allow reverting a root commit\n>   [3/3]: t3503: test cherry picking and reverting root commits\n\nJunio,\n\nI seem to recall seeing an email from you saying that merge-helpers need\nto handle tree-ish arguments, so this is an OK direction to go. But now\nI can't seem to find it. Did I dream it?\n\nIf that is the case, then I think this series is worth picking up. So I\nthought I'd prod you on it (I'm happy to repost, too, if that's easier).\n\n-Peff\n"},{"id":"167997","messageId":"7vwrhqlmeq.fsf@alter.siamese.dyndns.org","threadId":"27335","inReplyTo":"20110516102744.GA23286@sigill.intra.peff.net","subject":"Re: git cherry-pick --strategy=resolve segfaults if picking a root commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-16T15:01:33Z","receivedAt":"2011-05-16T15:01:33Z","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 Thu, May 12, 2011 at 07:08:55AM -0400, Jeff King wrote:\n>> The series is:\n>> \n>>   [1/3]: cherry-pick: handle root commits with external strategies\n>>   [2/3]: revert: allow reverting a root commit\n>>   [3/3]: t3503: test cherry picking and reverting root commits\n>\n> I seem to recall seeing an email from you saying that merge-helpers need\n> to handle tree-ish arguments, so this is an OK direction to go. But now\n> I can't seem to find it. Did I dream it?\n>\n> If that is the case, then I think this series is worth picking up. So I\n> thought I'd prod you on it (I'm happy to repost, too, if that's easier).\n\nNo you did not dream it but I think it is worth picking up. It was in my\n\"to look at\" queue that was accidentally got lost.\n\nThanks for reminding.\n"}]}