Re: [PATCHv2 1/2] merge: Add '--continue' option as a synonym for 'git commit'

From: Jeff King <peff@peff.net>
Date: Dec 13, 2016, 11:59 UTC
Message-ID: <20161213115931.tz7ce3z2meaxydbh@sigill.intra.peff.net>
In-Reply-To: <20161213084859.13426-1-judge.packham@gmail.com>

On Tue, Dec 13, 2016 at 09:48:58PM +1300, Chris Packham wrote:

Show 7 quoted lines

> +	if (continue_current_merge) {
> +		int nargc = 1;
> +		const char *nargv[] = {"commit", NULL};
> +
> +		if (argc)
> +			usage_msg_opt("--continue expects no arguments",
> +			      builtin_merge_usage, builtin_merge_options);

This checks that we don't have:

  git merge --continue foobar

but still allows:

  git merge --continue --some-option

because parse_options() decrements argc.

It would be insane to check individually which options might have been set. But I wonder if we could do something like:

  int orig_argc = argc;
  ...
  argc = parse_options(argc, argv, ...);

  if (continue_current_merge) {
	if (orig_argc != 1) /* maybe 2, to account for argv[0] ? */
		usage_msg_opt("--continue expects no arguments", ...);
  }

That gets trickier if there ever is an option that's OK to use with --continue. We might want to forward along "--quiet", for example. On the other hand, we silently ignore it now, so maybe it is better to complain and then let --quiet get added later if somebody cares.

Whatever we do here, I think "--abort" should get the same treatment (probably as a separate patch).

Show 11 quoted lines

> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
> index 85248a14b..44b34ef3a 100755
> --- a/t/t7600-merge.sh
> +++ b/t/t7600-merge.sh
> @@ -154,6 +154,7 @@ test_expect_success 'test option parsing' '
>  	test_must_fail git merge -s foobar c1 &&
>  	test_must_fail git merge -s=foobar c1 &&
>  	test_must_fail git merge -m &&
> +	test_must_fail git merge --continue foobar &&
>  	test_must_fail git merge
>  '

Your tests look good, though obviously if you check for options above, that should be covered in this test.

-Peff