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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: May 11, 2011, 12:59 UTC
Message-ID: <20110511125900.GH2676@elie>
In-Reply-To: <1305100822-20470-8-git-send-email-artagnon@gmail.com>

Ramkumar Ramachandra wrote:

Show 5 quoted lines

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

Show 18 quoted lines

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