{"thread":{"id":"35064","subject":"[PATCH] cherry-pick: do not segfault without arguments.","startedAt":"2013-10-04T14:09:12Z","lastAt":"2013-10-10T21:17:47Z","messageCount":3,"participants":["Stefan Beller","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"228559","messageId":"1380895752-5286-1-git-send-email-stefanbeller@googlemail.com","threadId":"35064","inReplyTo":null,"subject":"[PATCH] cherry-pick: do not segfault without arguments.","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-10-04T14:09:12Z","receivedAt":"2013-10-04T14:09:12Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Commit 182d7dc46b (2013-09-05, cherry-pick: allow \"-\" as abbreviation of\n'@{-1}') accesses the first argument without checking whether it exists.\n\nSigned-off-by: Stefan Beller \n---\n builtin/revert.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 52c35e7..f81a48c 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -202,7 +202,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n \tmemset(&opts, 0, sizeof(opts));\n \topts.action = REPLAY_PICK;\n \tgit_config(git_default_config, NULL);\n-\tif (!strcmp(argv[1], \"-\"))\n+\tif (argc > 1 && !strcmp(argv[1], \"-\"))\n \t\targv[1] = \"@{-1}\";\n \tparse_args(argc, argv, &opts);\n \tres = sequencer_pick_revisions(&opts);\n-- \n1.8.4.1.469.gb38b9db\n"},{"id":"228701","messageId":"20131010164116.GB21489@sigill.intra.peff.net","threadId":"35064","inReplyTo":"1380895752-5286-1-git-send-email-stefanbeller@googlemail.com","subject":"Re: [PATCH] cherry-pick: do not segfault without arguments.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-10-10T16:41:17Z","receivedAt":"2013-10-10T16:41:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 04, 2013 at 04:09:12PM +0200, Stefan Beller wrote:\n\n> Commit 182d7dc46b (2013-09-05, cherry-pick: allow \"-\" as abbreviation of\n> '@{-1}') accesses the first argument without checking whether it exists.\n\nI think this is an obviously correct fix for the immediate segfault,\nbut...\n\n> diff --git a/builtin/revert.c b/builtin/revert.c\n> index 52c35e7..f81a48c 100644\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -202,7 +202,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n> \tmemset(&opts, 0, sizeof(opts));\n> \topts.action = REPLAY_PICK;\n> \tgit_config(git_default_config, NULL);\n> -\tif (!strcmp(argv[1], \"-\"))\n> +\tif (argc > 1 && !strcmp(argv[1], \"-\"))\n> \t\targv[1] = \"@{-1}\";\n> \tparse_args(argc, argv, &opts);\n> \tres = sequencer_pick_revisions(&opts);\n\nWhy are we looking at argv/argc at all before calling parse_args? We\nwould want to allow options to come before the \"-\", no?\n\nIn other words, I think the right fix is more like this:\n\n-- >8 --\nSubject: [PATCH] cherry-pick: handle \"-\" after parsing options\n\nCurrently, we only try converting argv[1] from \"-\" into\n\"@{-1}\". This means we do not notice \"-\" when used together\nwith an option. We can fix this by doing the substitution\nafter we know any remaining options must be revisions.\n\nSince we now also come after the check that there is\nsomething in argv to cherry-pick, this has the added bonus\nof avoiding a segfault when \"git cherry-pick\" is run with no\noptions.\n\nNoticed-by: Stefan Beller \nSigned-off-by: Jeff King \n---\nI still am unsure if we should be more robust about finding \"-\" in other\noptions. For example, I think you can cherry-pick multiple commits these\ndays. Should we allow something like:\n\n git cherry-pick foo~2 - bar~5\n\nCertainly the convenience of \"-\" over \"@{-1}\" is not as big a deal if\nyou are cherry-picking more than one item, but it seems like we should\ntreat it consistently.\n\n builtin/revert.c | 4 ++--\n t/t3501-revert-cherry-pick.sh | 12 ++++++++++++\n 2 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 52c35e7..87659c9 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -168,6 +168,8 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \t\topts->revs->no_walk = REVISION_WALK_NO_WALK_UNSORTED;\n \t\tif (argc < 2)\n \t\t\tusage_with_options(usage_str, options);\n+\t\tif (!strcmp(argv[1], \"-\"))\n+\t\t\targv[1] = \"@{-1}\";\n \t\tmemset(&s_r_opt, 0, sizeof(s_r_opt));\n \t\ts_r_opt.assume_dashdash = 1;\n \t\targc = setup_revisions(argc, argv, opts->revs, &s_r_opt);\n@@ -202,8 +204,6 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n \tmemset(&opts, 0, sizeof(opts));\n \topts.action = REPLAY_PICK;\n \tgit_config(git_default_config, NULL);\n-\tif (!strcmp(argv[1], \"-\"))\n-\t\targv[1] = \"@{-1}\";\n \tparse_args(argc, argv, &opts);\n \tres = sequencer_pick_revisions(&opts);\n \tif (res < 0)\ndiff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh\nindex bff6ffe..51f3bbb 100755\n--- a/t/t3501-revert-cherry-pick.sh\n+++ b/t/t3501-revert-cherry-pick.sh\n@@ -129,4 +129,16 @@ test_expect_success 'cherry-pick \"-\" is meaningless without checkout' '\n \t)\n '\n \n+test_expect_success 'cherry-pick \"-\" works with arguments' '\n+\tgit checkout -b side-branch &&\n+\ttest_commit change actual change &&\n+\tgit checkout master &&\n+\tgit cherry-pick -s - &&\n+\techo \"Signed-off-by: C O Mitter \" >expect &&\n+\tgit cat-file commit HEAD | grep ^Signed-off-by: >signoff &&\n+\ttest_cmp expect signoff &&\n+\techo change >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.8.4.1.4.gf327177\n"},{"id":"228717","messageId":"5257197B.6010805@googlemail.com","threadId":"35064","inReplyTo":"20131010164116.GB21489@sigill.intra.peff.net","subject":"Re: [PATCH] cherry-pick: do not segfault without arguments.","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-10-10T21:17:47Z","receivedAt":"2013-10-10T21:17:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On 10/10/2013 06:41 PM, Jeff King wrote:\n> On Fri, Oct 04, 2013 at 04:09:12PM +0200, Stefan Beller wrote:\n> \n>> Commit 182d7dc46b (2013-09-05, cherry-pick: allow \"-\" as abbreviation of\n>> '@{-1}') accesses the first argument without checking whether it exists.\n> \n> I think this is an obviously correct fix for the immediate segfault,\n> but...\n\nYes my intention was to fix the obvious, as I assumed the discussion\nabout the exact spec for the '-' has been finished.\nYour patch probably makes more sense however.\n\n> \n>> diff --git a/builtin/revert.c b/builtin/revert.c\n>> index 52c35e7..f81a48c 100644\n>> --- a/builtin/revert.c\n>> +++ b/builtin/revert.c\n>> @@ -202,7 +202,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n>> \tmemset(&opts, 0, sizeof(opts));\n>> \topts.action = REPLAY_PICK;\n>> \tgit_config(git_default_config, NULL);\n>> -\tif (!strcmp(argv[1], \"-\"))\n>> +\tif (argc > 1 && !strcmp(argv[1], \"-\"))\n>> \t\targv[1] = \"@{-1}\";\n>> \tparse_args(argc, argv, &opts);\n>> \tres = sequencer_pick_revisions(&opts);\n> \n> Why are we looking at argv/argc at all before calling parse_args? We\n> would want to allow options to come before the \"-\", no?\n> \n> In other words, I think the right fix is more like this:\n> \n> -- >8 --\n> Subject: [PATCH] cherry-pick: handle \"-\" after parsing options\n> \n> Currently, we only try converting argv[1] from \"-\" into\n> \"@{-1}\". This means we do not notice \"-\" when used together\n> with an option. We can fix this by doing the substitution\n> after we know any remaining options must be revisions.\n> \n> Since we now also come after the check that there is\n> something in argv to cherry-pick, this has the added bonus\n> of avoiding a segfault when \"git cherry-pick\" is run with no\n> options.\n> \n> Noticed-by: Stefan Beller \n> Signed-off-by: Jeff King \n> ---\n> I still am unsure if we should be more robust about finding \"-\" in other\n> options. \n\nThere was a discusssion about it recently. - would generally mean the\nlast thing (kind of an undo), so git checkout - would switch to the last\nbranch. It's similar to 'cd -'.\nSo I am not sure if this is generally @{-1}, or if we need to have other\nreferences for other commands.\n\nFor example, I think you can cherry-pick multiple commits these\n> days. Should we allow something like:\n> \n> git cherry-pick foo~2 - bar~5\n\n\n\n> \n> Certainly the convenience of \"-\" over \"@{-1}\" is not as big a deal if\n> you are cherry-picking more than one item, but it seems like we should\n> treat it consistently.\n> \n> builtin/revert.c | 4 ++--\n> t/t3501-revert-cherry-pick.sh | 12 ++++++++++++\n> 2 files changed, 14 insertions(+), 2 deletions(-)\n> \n> diff --git a/builtin/revert.c b/builtin/revert.c\n> index 52c35e7..87659c9 100644\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -168,6 +168,8 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n> \t\topts->revs->no_walk = REVISION_WALK_NO_WALK_UNSORTED;\n> \t\tif (argc < 2)\n> \t\t\tusage_with_options(usage_str, options);\n> +\t\tif (!strcmp(argv[1], \"-\"))\n> +\t\t\targv[1] = \"@{-1}\";\n> \t\tmemset(&s_r_opt, 0, sizeof(s_r_opt));\n> \t\ts_r_opt.assume_dashdash = 1;\n> \t\targc = setup_revisions(argc, argv, opts->revs, &s_r_opt);\n> @@ -202,8 +204,6 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n> \tmemset(&opts, 0, sizeof(opts));\n> \topts.action = REPLAY_PICK;\n> \tgit_config(git_default_config, NULL);\n> -\tif (!strcmp(argv[1], \"-\"))\n> -\t\targv[1] = \"@{-1}\";\n> \tparse_args(argc, argv, &opts);\n> \tres = sequencer_pick_revisions(&opts);\n> \tif (res < 0)\n> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh\n> index bff6ffe..51f3bbb 100755\n> --- a/t/t3501-revert-cherry-pick.sh\n> +++ b/t/t3501-revert-cherry-pick.sh\n> @@ -129,4 +129,16 @@ test_expect_success 'cherry-pick \"-\" is meaningless without checkout' '\n> \t)\n> '\n> \n> +test_expect_success 'cherry-pick \"-\" works with arguments' '\n> +\tgit checkout -b side-branch &&\n> +\ttest_commit change actual change &&\n> +\tgit checkout master &&\n> +\tgit cherry-pick -s - &&\n> +\techo \"Signed-off-by: C O Mitter \" >expect &&\n> +\tgit cat-file commit HEAD | grep ^Signed-off-by: >signoff &&\n> +\ttest_cmp expect signoff &&\n> +\techo change >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n> \n"}]}