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

>>>>> 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. I see you mention that below.

Yuuuup, this one has been on my radar for quite some time ;D

    >> +default value. Likely, if an option is not defined in any configuration
    >> +file, the code should fallback to a default value.
    >> +
    >> +This also ensures compatibility with values provided via environment
    >> +variables of from the command line.

    > Though we should have this done in the config layer, not by random
    > higher level code. So we probably want something that will get a
    > value of a particular type, or warn and return a default if it doesn't
    > work.

/me nods. That's worth mentioning indeed.

<snip/>

    >> +Whatever path is used, the options apply if the branch path starts with
    >> +the path defining the section (or if the glob matches).

    > Are we going to also say in the section header what kind of section it
    > is, like [url bzr+ssh://launchpad.net/]?

    > It seems to me it would be nice, and is kind of implied by your
    > description, that the same sections can be used in any relevant file.
    > There may be snags with this approach though.

The current approach is to consider them as urls falling back to local
paths. I think we should reserve recognizing urls to locations.conf only
for overriding remote settings but I'm not clear about this particular
point.

    >> +
    >> +The ConfigOption object
    >> +-----------------------
    >> +
    >> +In addition to the configuration files, one internal configuration dict can
    >> +contain definitions for some configuration options. This will allow a finer
    >> +grained definition of the default values and the online help.
    >> +
    >> +The ConfigOption object will define:
    >> +
    >> +* name
    >> +
    >> +* default value. ``None`` is the "default" default value.
    >> +
    >> +* docstring used for the help
    >> +
    >> +* a list of config files where the option can be defined.

    > expected type: string, url, integer, bool, ...?

    > This is i guess where we would put the 'safety' thing, which may
    > overlap with specifying which files can contain it.

I'm really not sure about this one especially because I'd like to retain
the ability to recognized None as meaning: this hasn't been defined by
the user, but may be I'm not looking at it from the right perspective.

<snip/>

    >> +associated branch and repository object so only one of them is required (or
    >> +none for configuration files specific to the user like bazaar.conf and
    >> +locations.conf).

    > Some sample code would be good. Are people going to create a new
    > object every time they want to know a value? In some ways having
    > an implicit scope (or a session object) holding the configuration
    > for all relevant scopes could be good? Perhaps we can deal with
    > that when we cross it.

Yes, my current toughts are along this path (on both counts: the config
should come from the context (wt, branch, repo, session) and we'll deal
with it when we cross it ;)

<snip/>

    > I'd like to know where the project and organization files are
    > stored, but we can cross that later.

Yup, that's a pending question that I haven't answered yet: do we have a
pre-defined set of names/paths we check or do we allow arbitrary ones
and how are they specified (with an intersting bootstrap problem here as
defining them in bazaar.conf is way too late ;)

--C3596181551.1291278946/axe--