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