From: Jeff King <peff@peff.net>
Date: Tue, 13 Dec 2016 11:59:31 GMT
Subject: Re: [PATCHv2 1/2] merge: Add '--continue' option as a synonym for 'git commit'
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:

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

> 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