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

[Ramkumar Ramachandra <artagnon@gmail.com>]

Hi Jonathan,

Jonathan Nieder writes:
> 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.

What about --continue and --skip? They're no-ops too here, and
there'll soon be patches adding the functionality.  Do you think it's
alright to parse and exit immediately?

> > --- 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);

Huh? Why? I've caught every possible combination of two of those
options -- that already covers all three.

> ?  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);

I personally think having a function is cleaner: I even like the new
API suggested by Junio.  We can probably even move it to a common
place, and have others use it as well.

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

Over-engineering definitely! I'm looking to get something working
first; add-on functionality like this can come as later patches.

And yes, as you pointed out in another review, the name
verify_opt_incompatible_or_die is more appropriate.

Thanks for the detailed review.

-- Ram