Code review comment for lp:~vila/bzr/doc-new-config

>>>>> Andrew Bennetts <email address hidden> writes:

    > Review: Approve
    > As far as the proposed implementation goes, my thoughts are:

    > * there's lots of room for bikeshedding over syntax and even
    > features... but this seems reasonable to me

    > * I'm a bit worried about performance: we should make sure the
    > actual implementation imposes as little cost to the startup time
    > as possible. In particular I wonder about the costs of
    > evaluating definitions that refer to other definitions while
    > guarding against cycles etc.

Negligible, as long as it doesn't imply multiple IOs for the same config
file. This is not guaranteed by the current implementation as alluded in
the 'option life cycle' section.

Compared to parsing a config file, interpolating one option value should
also be negligible (this is easier to measure but requires using config
files with an "average" content to be ). Also note that the actual
implementation is using configobj which provides interpolation and is
already active by default, you can even use it today with the %(option)s
syntax.

    > It would be nice to have a configuration format that in principle
    > doesn't require any more data to be read, parsed and validated
    > than is relevant to the current operation.

That's a bit vague :)

My current thoughts are that we shouldn't try to validate the values
themselves, there are strings. period. Callers need to fallback to
default values if they can't find valid values in the files, which
includes the case where the option is not defined at all. As long as we
allow hand-editing of the config files, there is no guarantee that their
content is valid. From there, we can at best warn the user that some
content is invalid, but we can't block a bzr command because of that.

If we agree on that, then reading a config file can't fail and we can
focus on minimizing the number of times such a file should be read or
written.

Or may be you are referring to the edge case where we need to read all
config files in order to check that none of them define a given option ?
This is a concern with the actual implementation (see below) especially
if we start to use more or more config options (as in all the constants
used today in bzrlib).

    > * To a lesser extent I'm also concerned about the performance
    > impact of adding more kinds of config file. It'll probably be
    > fine, but we should measure.

So far, with the actual implementation, getting a config option value
requires reading one or several config files depending on the
context. Setting a new value requires reading and writing a config file
(whether or not we had already read it).

I'd like to change the implementation so that, during one bzrlib
"session" (hand waving, think of it as one bzr command), getting a
config value requires reading one or more config files but only once
(i.e. if we need more values, we won't read the same file
again). Setting a new value requires writing a config file (optionally
re-reading it for safety) but doing so only once by config file for
which at least one option has been modified.

So overall, we should issue the same number of IOs if a single option is
involved and *less* IOs if several options are involved.

Now, of course, if more config files are added, we will need to read
them, but that should scale better when multiple options are involved.

    > * How will we manage the transition from the current config files
    > to your proposed files?

Roughly, the files that exist today could still be used as is.

If you start using the new features, though, the old bzr versions won't
of course support them, this may be a concern for branch.conf, but
shouldn't be a problem for the local configurations files (bazaar.conf,
locations.conf). Using sections in branch.conf should be ignored by the
current implementation (I need to test this for locations.conf, but I'm
90% sure it will work). For branch.conf, this may be more problematic
since this requires synchronization across the branch users, but since
we don't have any feature relying on this yet, this shouldn't be an
immediate problem (and given that the expected usage of sections in
branch.conf (colo, loom, etc), I think it's a bit early to worry about
it since this will certainly requires other safe guards around their
usage).

Code-wise, the plan so far is too leave the old classes untouched and
deprecate them after a transition period (during 2.4 if this can land
for 2.3), allowing the plugins authors to migrate to the new scheme so
that plugin specific config classes used to implement plugin specific
config files will be supported.

About locations.conf being used to provide defaults, once sections are
handled into bazaar.conf, it should be a cut/paste from locations.conf
to bazaar.conf.

    > * I'd like more detail about what replacements for appendpath
    > will look like. Is there going to be a magic {rest-of-path]
    > interpolation value, or something like that?

Yes. I mentioned {relpath} in the 'Compatibility' section but I haven't
look precisely at what names should be used there (yet). Feedback
welcome.

    > * In general it would be good to have more examples, perhaps even
    > start the proposal with an example configuration file.

Indeed. I've started but we need far more. Writing the user guide should
help reach that point.

    > * I'm not sure how the proposed name spaces work: e.g. will users
    > need to write “bzr.debug_flags” rather than just “debug_flags”
    > everywhere?

Probably.

I had several related discussions with poolie and others about it on
IRC, I think the following alternatives were proposed:

- strong compliance with the bzrlib python namespace: if you use an
  option in bzrlib.xxx.yyy.zzz, the option abc should be named
  'bzrlib.xxx.yyy.zzz.abc'. That wasn't cool for plugins authors:
  'bzrlib.plugins.qbzr.diff.tool' is a bit long ;) So the consensus was to
  use just 'bzr.' and '<plugin_name>.' as prefixes.

- aliases: the idea is to allow shorter names either for options or
  prefixes à la selftest --starting-with. This may be confusing and
  was discarded.

This leaves the top-level quite free minus the plugin names. I vaguely
think we can use it for short names especially when providing additional
values for template interpolation (think merge tools command line
templates that need to be filled with {base}, {this}, {other},
etc). Using {bzr.merge.this}, {bzr.merge.other} may render templates
unreadable.

    > As far as landing this branch goes: I think land it. It should be
    > discussed on the list, but there's no reason not to land this
    > branch on trunk in the meantime.

Right, so poolie mentioned devnotes, I will land it there as a reference
point and will keep it up to date.

--F030E181551.1291278944/axe--