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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html