BUG: "git revert -m 0 <commit>" considered to be "git revert <commit>"

BUG: "git revert -m 0 <commit>" considered to be "git revert <commit>"

[Ævar Arnfjörð Bjarmason]

Both of these emit the "is a merge but no -m option was given" when
<commit> is a merge.

I tried to track this down for a bit in the options parsing code but
couldn't see where it was happening, but at some point we're setting
opts->mainline to 0 both when it's not provided, and when it's
explicitly provided as 0.

Instead we should e.g. pass "no option" through as NULL to the
sequencer, and emit some better error about how -m isn't zero-indexed.

[PATCH] cherry-pick: detect bogus arguments to --mainline

[Jeff King]

On Wed, Mar 15, 2017 at 12:08:35AM +0100, Ævar Arnfjörð Bjarmason wrote:

> Both of these emit the "is a merge but no -m option was given" when
> <commit> is a merge.
> 
> I tried to track this down for a bit in the options parsing code but
> couldn't see where it was happening, but at some point we're setting
> opts->mainline to 0 both when it's not provided, and when it's
> explicitly provided as 0.
> 
> Instead we should e.g. pass "no option" through as NULL to the
> sequencer, and emit some better error about how -m isn't zero-indexed.

The "0" comes from the initialization of the replay_opts struct (it also
happens if you explicitly disclaim any previous option with
--no-mainline).

I think using 0 as a sentinel is OK here, but the option-parser should
complain when we go out of range. Like this:

-- >8 --
Subject: [PATCH] cherry-pick: detect bogus arguments to --mainline

The cherry-pick and revert commands use OPT_INTEGER() to
parse --mainline. The stock parser is smart enough to reject
non-numeric nonsense, but it doesn't know that parent
counting starts at 1.

Worse, the value "0" is indistinguishable from the unset
case, so a user who assumes the counting is 0-based will get
a confusing message:

  $ git cherry-pick -m 0 $merge
  error: commit ... is a merge but no -m option was given.

Let's use a custom callback that enforces our range.

Signed-off-by: Jeff King <peff@peff.net>
---
Another option would be to add a range-checking variant of OPT_INTEGER,
and have something like:

  OPT_RANGE('m', "mainline", &opt->mainline, 1, INT_MAX,
            N_("parent number"))

I don't know if other places would want to make use of it, though. The
callback approach is way more flexible in general.

 builtin/revert.c             | 21 ++++++++++++++++++++-
 t/t3502-cherry-pick-merge.sh |  9 +++++++++
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 4ca5b5154..345d9586a 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -54,6 +54,24 @@ static int option_parse_x(const struct option *opt,
 	return 0;
 }
 
+static int option_parse_m(const struct option *opt,
+			  const char *arg, int unset)
+{
+	struct replay_opts *replay = opt->value;
+	char *end;
+
+	if (unset) {
+		replay->mainline = 0;
+		return 0;
+	}
+
+	replay->mainline = strtol(arg, &end, 10);
+	if (*end || replay->mainline <= 0)
+		return opterror(opt, "expects a number greater than zero", 0);
+
+	return 0;
+}
+
 LAST_ARG_MUST_BE_NULL
 static void verify_opt_compatible(const char *me, const char *base_opt, ...)
 {
@@ -84,7 +102,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
 		OPT_BOOL('e', "edit", &opts->edit, N_("edit the commit message")),
 		OPT_NOOP_NOARG('r', NULL),
 		OPT_BOOL('s', "signoff", &opts->signoff, N_("add Signed-off-by:")),
-		OPT_INTEGER('m', "mainline", &opts->mainline, N_("parent number")),
+		OPT_CALLBACK('m', "mainline", opts, N_("parent-number"),
+			     N_("select mainline parent"), option_parse_m),
 		OPT_RERERE_AUTOUPDATE(&opts->allow_rerere_auto),
 		OPT_STRING(0, "strategy", &opts->strategy, N_("strategy"), N_("merge strategy")),
 		OPT_CALLBACK('X', "strategy-option", &opts, N_("option"),
diff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh
index e37547f41..b1602718f 100755
--- a/t/t3502-cherry-pick-merge.sh
+++ b/t/t3502-cherry-pick-merge.sh
@@ -31,6 +31,15 @@ test_expect_success setup '
 
 '
 
+test_expect_success 'cherry-pick -m complains of bogus numbers' '
+	# expect 129 here to distinguish between cases where
+	# there was nothing to cherry-pick
+	test_expect_code 129 git cherry-pick -m &&
+	test_expect_code 129 git cherry-pick -m foo b &&
+	test_expect_code 129 git cherry-pick -m -1 b &&
+	test_expect_code 129 git cherry-pick -m 0 b
+'
+
 test_expect_success 'cherry-pick a non-merge with -m should fail' '
 
 	git reset --hard &&
-- 
2.12.0.613.g6e7c52a0d

Re: [PATCH] cherry-pick: detect bogus arguments to --mainline

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The "0" comes from the initialization of the replay_opts struct (it also
> happens if you explicitly disclaim any previous option with
> --no-mainline).
>
> I think using 0 as a sentinel is OK here, but the option-parser should
> complain when we go out of range. Like this:

An alternative could be to use -1 as a sentinel and clean it up
after parse_options() returns, but then we will see another "bug"
report that says "cherry-pick -m -1" gives a bad error message,
which essentially is the same "bug" report as the one that triggered
this change, which says the error message from "cherry-pick -m 0" is
wrong X-<.

The "RANGE" thing is certainly tempting, but let's do so after we
find multiple places that can benefit from it.

Will queue; thanks.