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

Ævar Arnfjörð Bjarmason· Mar 14, 2017, 23:08 UTC · lore

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.

Jeff King· Mar 15, 2017, 16:56 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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

Show 10 quoted lines

> 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

Junio C Hamano· Mar 15, 2017, 19:15 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:

Show 6 quoted lines

> 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.