Re: [Monotone-devel] nvm.options

[Thomas Keller]

Am 09.08.10 01:49, schrieb 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?

Yes, they help indeed. I slowly get a clue and thats a good thing :)

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

Excellent explanation. I've added it in
96917638555653c858055bc810072c30034e82e1 just in case somebody else
stumbles upon that.

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

Understood, unfortunately we do not have this common naming everywhere,
e.g. the path types come into my mind, but maybe we should list this in
HACKING to make the naming semi-official at least for new development...?

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

Ok cool. The "-2" might look a bit weird, but I actually find the
explanation that the default verbosity is denoted with 0 quite elegant
and therefor defendable on user inquiries :)

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

Very nice. This looks much better now, though I guess some people might
not like these long option names. I guess discussions will up when this
lands and is released in 0.99. Thank god we're then still one release
away from 1.0 to calm the waves... maybe we can also come up with some
smart solution by then, like accepting partial option names just what we
do with command names? The easiest would probably be to match the common
prefix, so --no-st could automatically match to --no-standard-rcfiles if
there are no ambigiuous options or bail out possible expansions. From
what I see in the code, it should be quite easy to do in getopt() in
option.cc, right? Lets see if I can accomplish that...

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

Thanks!

Thomas.

-- 
GPG-Key 0x160D1092 | address@hidden | http://thomaskeller.biz
Please note that according to the EU law on data retention, information
on every electronic information exchange might be retained for a period
of six months or longer: http://www.vorratsdatenspeicherung.de/?lang=en

signature.asc
Description: OpenPGP digital signature