Re: [PATCH 7/8] revert: Implement parsing --continue, --abort and --skip

[Jonathan Nieder <jrnieder@gmail.com>]

Ramkumar Ramachandra wrote:

> Introduce three new command-line options: --continue, --abort, and
> --skip resembling the correspoding options in "rebase -i".  For now,
> just parse the options into the replay_opts structure, making sure
> that two of them are not specified together. They will actually be
> implemented later in the series.

I'd suggest squashing this patch with the next one.  If a "git
cherry-pick" accepting an --abort option that does not do anything
leaked into the wild, that would not be a good outcome.

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -145,7 +153,47 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
>  	opts->xopts_nr = xopts_nr;
>  	opts->xopts_alloc = xopts_alloc;
>  
> -	if (opts->commit_argc < 2)
> +	/* Check for incompatible command line arguments */
> +	if (opts->abort_oper || opts->skip_oper || opts->continue_oper) {
> +		char *this_oper;
> +		if (opts->abort_oper) {
> +			this_oper = "--abort";
> +			die_opt_incompatible(me, this_oper,
> +					"--skip", opts->skip_oper,
> +					NULL);
> +			die_opt_incompatible(me, this_oper,
> +					"--continue", opts->continue_oper,
> +					NULL);

What happened to

			...(me, "--abort",
				"--skip", opts->skip,
				"--continue", opts->continue);

?  I also wonder if there should not be a function to deal with
mutually incompatible options:

	va_start(ap, commandname);
	while ((arg1 = va_arg(ap, const char *))) {
		int set = va_arg(ap, int);
		if (set)
			break;
	}
	while ((arg2 = va_arg(ap, const char *))) {
		int set = va_arg(ap, int);
		if (set)
			die(arg1 and arg2 are incompatible);
	}
	va_end(ap);

> +		die_opt_incompatible(me, this_oper,
> +				"--no-commit", opts->no_commit,
[...]

Seems reasonable.  A part of me would want to accept such options and
only error out if the saved state indicates that they are different
from the options supplied before, so if a person has

	alias applycommits = git cherry-pick --no-commit

then "applycommits --continue" could work without trouble, but
that's probably overegineering.