Merge lp:~vila/bzr/doc-new-config into lp:bzr

I've started looking at this. So far I can say: that is a clear and comprehensive list of the current issues. Thank you!

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. 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.
 * 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.
 * How will we manage the transition from the current config files to your proposed files?
 * 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?
 * In general it would be good to have more examples, perhaps even start the proposal with an example configuration file.
 * 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?

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.

Overall:

I'm ok to land it. I would somewhat prefer to put it into devnotes
rather than trunk, because it is more of working notes than
documentation, though I have to admit devnotes has not been very
active.

I agree with spiv that seeing some examples of the configuration files
and APIs would probably help a lot.

It seems like there are various aspects that can be tackled
semi-independently though with ordering constraints, perhaps starting
with the new api for getting configuration values.

On 13 November 2010 02:18, Vincent Ladeuil <email address hidden> wrote:
> Vincent Ladeuil has proposed merging lp:~vila/bzr/doc-new-config into lp:bzr with lp:~vila/bzr/672382-list-values as a prerequisite.
>
> Requested reviews:
>  bzr-core (bzr-core)
>
>
> This is a brain dump of some thoughts about a new configuration option handling implementation.
>
> Any kind of feedback welcome :)
>
> I'd like to better separate this document into a user oriented one and a developer oriented one in the future so help welcome on this too.

I'm glad you wrote this up.

I wonder if "here are problems we have now and proposed solutions"
should really live in the main tree, but it's better to have it there
than nowhere. In particular, the 'current' bit rarely seems to get
updated when things fixing it land.

Perhaps some of this can be recycled into user documentation or
developer documentation as the features it describes land.

One further issue is that some values seem to be safe to take from
untrustworthy data, eg remote or in-tree configuration (eg branch
nick), and others are not (eg a hook to run). I have been wondering
if we ought to make this part of the declaration of a hook, but I'm
not sure if a simple safe/unsafe split is enough.

> +Not all needs can be addressed by the default values used inside bzr and
> +bzrlib, no matter how well they are chosen (and they are ;).
> +
> +Many parts of ``bzrlib`` depends on some constants though and the user
> +should be able to customize the behavior to suit his needs so these
> +constants need to become configuration options.
> +
> +These options can be set from the command-line, in an environment variable
> +or recorded in a configuration file.
> +
> +Current issues
> +==============

I guess all of these, except perhaps the first (which is general)
should have bugs, but there may be little point manually pasting them
here.

> +
> +* Many parts of ``bzrlib`` declare constants and there is no way for the
> +  user to look at or modify them.
> +
> +* The current API requires a configuration object to create, modify or
> +  delete a configuration option in a given configuration file.  ``bzr
> +  config`` makes it almost transparent for the user. Internally though, not
> +  all cases are handled: only BranchConfig implements chained configs,
> +  nothing is provided at the repository level and too many plugins define
> +  their own section or even their own config file.

The fact that you need an object is not a problem (afaics); the fact
that we have one class per place they can be stored is distasteful.

> +
> +* ``locations.conf`` defines the options that needs to override any setting
> +  in ``branch.conf`` fo...

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

>>>>> Martin Pool <email address hidden> writes:

    > Overall:
    > I'm ok to land it. I would somewhat prefer to put it into devnotes
    > rather than trunk, because it is more of working notes than
    > documentation,

I'll do that.

    > though I have to admit devnotes has not been very active.

    > I agree with spiv that seeing some examples of the configuration files
    > and APIs would probably help a lot.

Sure, I'll keep working on it to turn it into proper documentation. I
wanted some rough agreement before going further and try to split it
into work items.

    > It seems like there are various aspects that can be tackled
    > semi-independently though with ordering constraints, perhaps
    > starting with the new api for getting configuration values.

I'm still unclear of the path to follow from here, my next target was a
first step in interpolation limited in scope to values defined in a
single config file.

<snip/>

    > I wonder if "here are problems we have now and proposed solutions"
    > should really live in the main tree, but it's better to have it there
    > than nowhere. In particular, the 'current' bit rarely seems to get
    > updated when things fixing it land.

Right, I try to address that by keeping it in the upper thread in my
loom, so that after I land something I keep coming to it.

    > Perhaps some of this can be recycled into user documentation or
    > developer documentation as the features it describes land.

That's the idea, I may even start with that before implementing anything.

    > One further issue is that some values seem to be safe to take from
    > untrustworthy data, eg remote or in-tree configuration (eg branch
    > nick), and others are not (eg a hook to run). I have been
    > wondering if we ought to make this part of the declaration of a
    > hook, but I'm not sure if a simple safe/unsafe split is enough.

Me neither, one thing I haven't done yet, it to list all the
actual/potential config options to get a better idea.

<snip/>

    >> +Current issues
    >> +==============

    > I guess all of these, except perhaps the first (which is general)
    > should have bugs, but there may be little point manually pasting
    > them here.

I've tried to mention the ones I was aware of in the text.

<snip/>

    > The fact that you need an object is not a problem (afaics); the fact
    > that we have one class per place they can be stored is distasteful.

Yup. The goal is that the same object should be usable in all cases and
ideally already provided in most of them (via the wt, branch, repo, etc
(hand-waving on the etc;)).

<snip/>

    >> This is not a big concern so far as very few needs required
    >> dicts as option values.

    > last sentence doesn't quite parse.

'gtk_file_commit_messages' in branch.conf use a bencoded value to store
a dict in the branch.conf file. I'm not sure I know other cases.

Is:

  In a few known cases, a bencoded dict is stored in a config value, so
  while this isn't user-friendly, not providing a better alternative
  shouldn't be a concern.

clearer ?

    > One more, for which there is a bug, is wanting a -O per-process config
    > setting. ...

Landed in lp:~bzr-core/bzr/devnotes/.

Keep replying by mail if you have further comments though !