{"thread":{"id":"45378","subject":"BUG: \"git revert -m 0 \" considered to be \"git revert \"","startedAt":"2017-03-14T23:09:01Z","lastAt":"2017-03-15T19:16:19Z","messageCount":3,"participants":["Ævar Arnfjörð Bjarmason","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"314105","messageId":"CACBZZX7Um==4pWfUVcLM8Kbk-ry8HQAmDoUVGjVPmhY3ZXUNYg@mail.gmail.com","threadId":"45378","inReplyTo":null,"subject":"BUG: \"git revert -m 0 \" considered to be \"git revert \"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-14T23:08:35Z","receivedAt":"2017-03-14T23:09:01Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Both of these emit the \"is a merge but no -m option was given\" when\n is a merge.\n\nI tried to track this down for a bit in the options parsing code but\ncouldn't see where it was happening, but at some point we're setting\nopts->mainline to 0 both when it's not provided, and when it's\nexplicitly provided as 0.\n\nInstead we should e.g. pass \"no option\" through as NULL to the\nsequencer, and emit some better error about how -m isn't zero-indexed.\n"},{"id":"314145","messageId":"20170315165623.sniukjnhonf3uddy@sigill.intra.peff.net","threadId":"45378","inReplyTo":"CACBZZX7Um==4pWfUVcLM8Kbk-ry8HQAmDoUVGjVPmhY3ZXUNYg@mail.gmail.com","subject":"[PATCH] cherry-pick: detect bogus arguments to --mainline","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-15T16:56:23Z","receivedAt":"2017-03-15T16:56:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 15, 2017 at 12:08:35AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> Both of these emit the \"is a merge but no -m option was given\" when\n> is a merge.\n> \n> I tried to track this down for a bit in the options parsing code but\n> couldn't see where it was happening, but at some point we're setting\n> opts->mainline to 0 both when it's not provided, and when it's\n> explicitly provided as 0.\n> \n> Instead we should e.g. pass \"no option\" through as NULL to the\n> sequencer, and emit some better error about how -m isn't zero-indexed.\n\nThe \"0\" comes from the initialization of the replay_opts struct (it also\nhappens if you explicitly disclaim any previous option with\n--no-mainline).\n\nI think using 0 as a sentinel is OK here, but the option-parser should\ncomplain when we go out of range. Like this:\n\n-- >8 --\nSubject: [PATCH] cherry-pick: detect bogus arguments to --mainline\n\nThe cherry-pick and revert commands use OPT_INTEGER() to\nparse --mainline. The stock parser is smart enough to reject\nnon-numeric nonsense, but it doesn't know that parent\ncounting starts at 1.\n\nWorse, the value \"0\" is indistinguishable from the unset\ncase, so a user who assumes the counting is 0-based will get\na confusing message:\n\n $ git cherry-pick -m 0 $merge\n error: commit ... is a merge but no -m option was given.\n\nLet's use a custom callback that enforces our range.\n\nSigned-off-by: Jeff King \n---\nAnother option would be to add a range-checking variant of OPT_INTEGER,\nand have something like:\n\n OPT_RANGE('m', \"mainline\", &opt->mainline, 1, INT_MAX,\n N_(\"parent number\"))\n\nI don't know if other places would want to make use of it, though. The\ncallback approach is way more flexible in general.\n\n builtin/revert.c | 21 ++++++++++++++++++++-\n t/t3502-cherry-pick-merge.sh | 9 +++++++++\n 2 files changed, 29 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 4ca5b5154..345d9586a 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -54,6 +54,24 @@ static int option_parse_x(const struct option *opt,\n \treturn 0;\n }\n \n+static int option_parse_m(const struct option *opt,\n+\t\t\t const char *arg, int unset)\n+{\n+\tstruct replay_opts *replay = opt->value;\n+\tchar *end;\n+\n+\tif (unset) {\n+\t\treplay->mainline = 0;\n+\t\treturn 0;\n+\t}\n+\n+\treplay->mainline = strtol(arg, &end, 10);\n+\tif (*end || replay->mainline <= 0)\n+\t\treturn opterror(opt, \"expects a number greater than zero\", 0);\n+\n+\treturn 0;\n+}\n+\n LAST_ARG_MUST_BE_NULL\n static void verify_opt_compatible(const char *me, const char *base_opt, ...)\n {\n@@ -84,7 +102,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n \t\tOPT_BOOL('e', \"edit\", &opts->edit, N_(\"edit the commit message\")),\n \t\tOPT_NOOP_NOARG('r', NULL),\n \t\tOPT_BOOL('s', \"signoff\", &opts->signoff, N_(\"add Signed-off-by:\")),\n-\t\tOPT_INTEGER('m', \"mainline\", &opts->mainline, N_(\"parent number\")),\n+\t\tOPT_CALLBACK('m', \"mainline\", opts, N_(\"parent-number\"),\n+\t\t\t N_(\"select mainline parent\"), option_parse_m),\n \t\tOPT_RERERE_AUTOUPDATE(&opts->allow_rerere_auto),\n \t\tOPT_STRING(0, \"strategy\", &opts->strategy, N_(\"strategy\"), N_(\"merge strategy\")),\n \t\tOPT_CALLBACK('X', \"strategy-option\", &opts, N_(\"option\"),\ndiff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh\nindex e37547f41..b1602718f 100755\n--- a/t/t3502-cherry-pick-merge.sh\n+++ b/t/t3502-cherry-pick-merge.sh\n@@ -31,6 +31,15 @@ test_expect_success setup '\n \n '\n \n+test_expect_success 'cherry-pick -m complains of bogus numbers' '\n+\t# expect 129 here to distinguish between cases where\n+\t# there was nothing to cherry-pick\n+\ttest_expect_code 129 git cherry-pick -m &&\n+\ttest_expect_code 129 git cherry-pick -m foo b &&\n+\ttest_expect_code 129 git cherry-pick -m -1 b &&\n+\ttest_expect_code 129 git cherry-pick -m 0 b\n+'\n+\n test_expect_success 'cherry-pick a non-merge with -m should fail' '\n \n \tgit reset --hard &&\n-- \n2.12.0.613.g6e7c52a0d\n\n"},{"id":"314158","messageId":"xmqq4lyufj1b.fsf@gitster.mtv.corp.google.com","threadId":"45378","inReplyTo":"20170315165623.sniukjnhonf3uddy@sigill.intra.peff.net","subject":"Re: [PATCH] cherry-pick: detect bogus arguments to --mainline","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-15T19:15:44Z","receivedAt":"2017-03-15T19:16:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The \"0\" comes from the initialization of the replay_opts struct (it also\n> happens if you explicitly disclaim any previous option with\n> --no-mainline).\n>\n> I think using 0 as a sentinel is OK here, but the option-parser should\n> complain when we go out of range. Like this:\n\nAn alternative could be to use -1 as a sentinel and clean it up\nafter parse_options() returns, but then we will see another \"bug\"\nreport that says \"cherry-pick -m -1\" gives a bad error message,\nwhich essentially is the same \"bug\" report as the one that triggered\nthis change, which says the error message from \"cherry-pick -m 0\" is\nwrong X-<.\n\nThe \"RANGE\" thing is certainly tempting, but let's do so after we\nfind multiple places that can benefit from it.\n\nWill queue; thanks.\n"}]}