Re: [Monotone-devel] overwriteable and negatable options

[Stephen Leake]

Timothy Brownawell <address@hidden> writes:

> On 06/13/2010 07:11 AM, Stephen Leake wrote:
>> Timothy, I noticed you edited the RoadMap wiki to say overwriteable and
>> negatable options is started on branch nvm.options. But I don't see that
>> branch yet.
>>
>> I'd like to work on this as well. Can you push your branch, and outline
>> your plan?
>
> It is pushed, and has been for a day or so (with varying amounts done).

Yes, I see it now, and I've read thru the changes and the code. It's
very impressive work.

> What I did is extend the flag syntax. Where it could take "foo,f" to
> accept "--foo" and "-f" flags, it can now take "foo,f/no-foo" and then
> "--no-foo" will reset the optset that the flag is attached to. This
> allows for example "--rcfile" to also have "--clear-rcfiles" which
> drops any eariler "--rcfile" arguments, besides just simple boolean
> options being resettable.

Right. This is more than I was proposing, but makes a lot of sense.

> I guess it should be possible to make the parser also recognize
> added/missing 'no-' prefixes on argument-free options, but this has
> the issue of the parser not knowing what variables get set by the
> option, and probably not wanting to implicitly add ways to reset
> optsets that may have other things in them (and what should
> "--no-content", "--no-unified", and "--no-external" do? As far as the
> parser is concerned "--context", "--unified", and "--external" look
> just like real boolean options due to having no argument, even they go
> into the same 3-way enum.).

Right. Your mechanism is much more flexible. It requires specifying the
cancel arg for each option that needs it, but that's a very small price
to pay.

> Additionally, many of our option setter bodies either just set a bool,
> stored the argument, or added the argument to a collection. So for the
> case like this where one flag goes with one variable (ie, things
> suited for OPT or GOPT with very boring setter bodies) I added
> SIMPLE_OPTION and GLOBAL_SIMPLE_OPTION macros that use template magic
> to figure out the right setter body.

Right; that reduces the amount of work needed to add a cancelable
option.

> Many options are updated to use the simpler macros and reset flags,
> some aren't yet.

Ok. 

> I've also rearranged how options are applied, now they are always done
> in order of precedence instead of worrying about whether a particular
> mechanism should ignore already-set options:
>   1.  apply any options from hook_get_default_command_options, for
>       the command run on the command line
>   1a. if stdio, mtn_automate, or network automate, apply any
>       options from the hook_get_default_command_options for the
>       current automate command
>   2.  apply workspace options, if relevant to the command being run
>   3.  apply options from the command line
>   3a. if stdio, mtn_automate, or network automate, apply any
>       options given with the command
>
> So putting "--update" in the default options hook and "--no-update" on
> the command line, or anything similar to this, will work as expected
> now.

Right. This code is much cleaner, and more unified.

>> I think there are two separate parts to this:
>>
>> 1) enhance the options parser (option.cc from_command_line) to support
>>     --[no]- for boolean options
>>
>>      1a) rename all current boolean options that start with 'no' or
>>          equivalent
>
> Even when the option is to turn something off that's done by default
> ("--no-graph" on log)?

If we were starting from scratch I'd have defined an option '--graph',
with the default value of 'true'. Then --no-graph would turn it off.

But your way is a smaller change to the current code.

>>      1b) add a deprecation method for options that now have better names
>
> Hm. Probably the way to do this would be to be able to mark an option
> as hidden, like we can have hidden commands.

With your cancel mechanism, we don't have any deprecated options, so
this can wait for another time.

>> 2) ensure that all methods for getting options support overwriting
>>     previous settings.
>
> This is done with the application order changes.

Right.

>> I've started reading thru options.cc. I'm currently puzzled why there
>> are explicit option bodies in options_list.hh for bool options; it seems
>> the setter class should do that automatically. I think we'll need that
>> for --no-* to work.
>
> Well, there are two things. All flags have a "${foo}_given" bool
> (which generally aren't used much), and then the actual variable that
> is used is "${foo}" and often is identical to "${foo}_given" for bools
> but may be reversed.
>
> Hm. One thing with how I set up reset flags, is that by resetting the
> optset they also reset the _given flags. This means there's no
> difference at all between "--update --no-update", "--no-update", and
> not giving either. I assume this is what we want, if we gave --key a
> --no-key (better named --default-key or something) option by this
> mechanism it would have different results than giving "--key ''" (fall
> back to hooks or seeing if we have only one key, rather than
> explicitly not using a key).

Yes, I think this will be ok.

>> OPTVAR(diff_options, bool, without_header, false);
>> OPTVAR(diff_options, bool, with_header, false);
>>
>> we could keep both, but deprecate 'without-header' and eventually remove
>> it. That requires adding an option deprecation method, that prints a
>> warning when users use a deprecated option.
>
> These are fun as the default for 'diff' is --with-header and the
> default for 'automate content_diff' is --without-header.

And I see you have left them separate for now. 

I had not anticipated different defaults for different commands.

We could handle that with a default standard hook for
get_default_command_options. But that would break if people currently
define that hook but don't add the new code.

We could add other code that is run before the user
get_default_command_options that provides the same mechanism, and have
it set these appropriately. That could be useful in the long run, for
shared options on other commands.

>> If we have a deprecation method, we could keep "norc", "nostd", and
>> "non-interactive" as deprecated options as well.
>
> Right now I have them paired with --yesrc and --stdhooks .

Yes, I like that.

> We also have --unknown for add, and a completely separate --no-unknown
> for inventory. This is also fun due to wanting the default to be
> different depending on which command is being run.

I think it's ok to have disjoint options with similar names, as long as
they are used in different commands. 'diff' and 'automate content_diff'
are very similar commands, so I think we should try to fix that and
unify those options. I'd be ok with leaving "unknown" and "no-unknown"
alone. But if we adopt the std_get_default_command_options approach,
that could used to fix both.

I'm getting a link error (Debian, g++ 4.4.3, libboost-all-dev 1.42.0.1):

roster.o: In function `void dump<unsigned int>(unsigned int const&, 
std::basic_string<char, std::char_traits<char>, std::allocator<char> >&)':
/home/Projects/monotone/monotone.options-build/../monotone.options/roster.cc:66:
 multiple definition of `void dump<unsigned int>(unsigned int const&, 
std::basic_string<char, std::char_traits<char>, std::allocator<char> >&)'
cmd.o:/home/Projects/monotone/monotone.options-build/../monotone.options/cmd.cc:91:
 first defined here

I don't see what the problem is, nor how to fix it.

I'll try on Win32 MinGW.
 
-- 
-- Stephe