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

[Jonathan Nieder <jrnieder@gmail.com>]

Ramkumar Ramachandra wrote:

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

You're right: the same considerations apply to them.  If adding these
options before the functionality is ready makes the series easier to
read, then I'd at least prefer to see

	if (opts->abort_oper)
		die("--abort is not implemented yet");

to prevent scripts and humans from being confused.  And on the other
hand I suspect adding each option at the same time as adding the
corresponding functionality would be clearer anyway.

> Jonathan Nieder writes:
>> Ramkumar Ramachandra wrote:

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

Sorry, that was unclear of me.  What I meant to say is that one
function call instead of two would suffice, like the API is
supposed to make possible.

In other words, nothing actually wrong here, just a possibility
of simplification.

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

Sorry, I was unclear again.  What I meant is that there could be
two functions:

 - one to check a single option against various options it is
   incompatible with, which you've already written
 - another to check a family of mutually incompatible options

The above was a sample implementation for the second function, but it
has a bug: the second "while" loop should have been preceded by
"if (!arg1) return;".

>> 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
[...]
> Over-engineering definitely!

Yep, sorry.  Was just thinking out loud.