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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: May 13, 2011, 09:40 UTC
Message-ID: <20110513094038.GA30396@elie>
In-Reply-To: <20110513091619.GC14272@ramkum.desktop.amazon.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)

[...]

Show 15 quoted lines

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

Show 17 quoted lines

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