Re: [Monotone-devel] nvm.options

[Timothy Brownawell]

On 07/20/2010 06:10 PM, Thomas Keller wrote:

> I wished that you'd have commented a few places a bit more though for us
> other devs so we properly use the new / changed macros
> (CMD_PRESET_OPTIONS is not mentioned f.e.) and have an idea what certain
> code paths do (just out of my head here, the `check_by_name_insertion'
> function in option.cc, but there are others as well). I cannot quite
> follow when OPTSET_REL is needed and when not and I see that some
> options are grouped in an optset and some aren't (which look as they
> belong together somehow as well, like the options for mtn log, or all
> the single options which control the export format for git_export).

I've added some comments on these, do they help at all?

> Minor other things I noticed on the way:
>
> - work.cc (get_options): `opts.key_dir_given = true;` is commented out,
>    is the given-ness of an option now implicit so that this is no longer
>    needed? My original drive here for the code was to let an options
>    instance which got instantiated from _MTN/options look equal to an
>    options instance which got instantiated from command line or
>    elsewhere, so if keydir is present in _MTN/options, its surely "given"
>    in the option instance - or what do you say?

This is due to the interaction between --confdir and --keydir. If 
--keydir is not given and --confdir is, then --keydir will default to 
the "keys" subdirectory of the given confdir. This works by the 
--confdir option body looking at key_dir_given; if reading the keydir 
from _MTN/options set that, then --confdir would stop setting the 
default keydir when in a workspace.

> - option.hh: minor nitpick, there are two enums, one "option_parse_type"
>    and one "allow_xargs_t", should the former be rather named
>    "option_parse_t" to match the latter and other occurences or what is
>    the point of suffixing "_t" at all? (Excuse my ignorance, I just
>    never saw that anywhere else...)

This is cleaned up now.

And "_t" is a moderately-common suffix for typenames. In particular it 
can help slightly when the reasonable typename is also a reasonable 
variable name, for example "size_t size" might be a bit of a pain to 
read if the type didn't have the "_t" at the end.

> - options_list.hh still mentions OPT / GOPT in comments, comment on top
>    notes "4 important macros", but there are now 6

This should be fixed now.

> - now that we have a very fine-grained verbosity control - do we want
>    to keep --quient and --reallyquiet as UI sugar or should we deprecate
>    them as well for 2.0?

I think --quiet is somewhat standard across programs and should probably 
stay just for that (same for --debug). --reallyquiet is deprecated in 
favor of --verbosity=-2.

> - could we find a better cancel name for "norc/yesrc" or make "norc"
>    deprecated and introduce "load-rc/no-load-rc" instead? Maybe we
>    should name them even "load-rc-files/no-load-rc-files", since this
>    would be orthogonal to the other existing option "rcfile", which,
>    otherwise, should probably be just named "rc".
>    Same with "nostd" and others I missed - lets move forward and
>    deprecate these misleading / non-standard option names. If the options
>    get too long for a simple and quick usage, lets introduce more
>    short versions for them.

These are now --no-standard-rcfiles/--standard-rcfiles and 
--no-builtin-rcfile/--builtin-rcfile , with --nostd/--norc marked as 
deprecated.

> - options.hh (enum_string): I couldn't test this because of the above
>    problems, but the code which checks for allowed values does not look
>    like it would honor partial strings, i.e. on a enum_string "foo,bar"
>    it should bail out if only "fo" or "ar" is given, but currently does
>    not, is that right?

Yep, this should be fixed now.

--
Timothy

Free public monotone hosting: http://mtn-host.prjek.net