Re: [Monotone-devel] nvm.options

[Thomas Keller]

Am 20.07.10 04:58, schrieb Timothy Brownawell:
> On 07/18/2010 08:19 AM, Stephen Leake wrote:
>> Tim,
>>
>> Just letting you know I've been keeping up with nvm.options, and it all
>> looks very good so far.
> 
> I think it's ready to merge now. See anything I missed?

The current head (44c1dd7bbc276790c51463a83823d2e1339c7ff6) fails to
compile for me:

base.hh: In function ‘void dump(const T&, std::string&) [with T = long
unsigned int]’:
sanity.hh:451:   instantiated from ‘void Musing<T>::gasp(std::string&)
const [with T = size_t]’
cmd.cc:640:   instantiated from here
base.hh:45: error: invalid application of ‘sizeof’ to incomplete type
‘dump_must_be_specialized_for_this_type’
make[1]: *** [cmd.o] Error 1
make: *** [all] Error 2

Other than that, tremendous work again from what I see in the code! I
particularily like the functionality to hide "internal" options like
bind_stdio, so they stop cluttering the namespace and do not confuse the
user. The SIMPLE_OPTION macro with its support for enumerated values and
sets and of course simple types makes it far easier to add new options.
Finally, the unification of the option handling / command processing for
automate stdio and friends is also very welcome, though its a bit out of
scope for this branch.

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). And
I miss a NEWS entry which tells the user about the option changes, i.e.
which options got deprecated, which got removed, which got replaced,
which got hidden, aso.

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?
- 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...)
- options_list.hh still mentions OPT / GOPT in comments, comment on top
  notes "4 important macros", but there are now 6
- 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?
- 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.
- 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?

If the compilation issue is fixed, I'll check the UI again and may come
up with other little things :)

Thanks for your work so far!
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