Code review comment for lp:~doxxx/bzr/mergetools

>>>>> Gordon Tyler <email address hidden> writes:

    > On Thu, November 11, 2010 1:19 pm, Vincent Ladeuil wrote:
    >> > I'm not sure how I would actually go about splitting it. Branch from
    >> > lp:~doxxx/bzr/mergetools and then remove the extra bits from
    >> > lp:~doxxx/bzr/mergetools?
    >>
    >> Whatever works :) The above is ok. Then you'll have to decide whether
    >> you want to regularly merge from one branch to the other, use a pipeline
    >> or a loom. And don't forget to set the pre-requisite branch when
    >> submitting.

    > Won't the removal of extra bits in the original get merged into the second
    > when I try and merge new changes in the original into the second?

You have to create the second branch on top of the first one of course.

    > I already create option names like that but I didn't see a way to
    > query for options whose names start with a prefix, thus the
    > separate list option. I'll take another look at the config stuff
    > to see how to do this.

Look at the cmd_config implementation, _show_matching_options implements
such a filtering (with name='^bzr.mergetool.'), if you need it we should
certainly plan to make it available in a form reusable for your use
case, unless we provide a way to access such a set of options as a dict
(this will complicate the model (as in: should the dict be defined for
*each* config file OR be the union of the definitions in all config
files or something else)) but I think it's too soon to wander into such
complications now.

    > But yeah, if I can get this working then it does become more
    > user-editable, in which case I would be okay with removing the --add,
    > --update and --remove options from the mergetools command.

Cool.

    > Should these options go into a different section in the config
    > file? Or would that prevent putting them in locations.conf and
    > branch.conf?

Yes, using sections for that conflicts with using section names for
paths (see my other comment).

    > What about tool names which have "funny" characters in them like
    > spaces or other stuff? What's legal in an option name?

I haven't checked precisely what is legal and what is not, but python
identifiers are :) So no spaces, no funny names. I don't think it's a
problem.

<snip/>

    >> >> - _KOWN_MERGE_TOOLS really want to be a registry :)
    >>
    >> > I'll have a look at that.
    >>
    >> May be not :) If you rely on a naming rule for that...

    > It's just a place to list the names and command-lines of merge
    > tools that we already know about, so that we can try detect them
    > on the path. Is a registry suitable for that?

A registry will give you a place to store the default values that you
don't want to put in bazaar.conf and *this* is not implement today in
bzrlib.config.I'd like to have a config-like object that is *not*
associated to a file on disk but to a set of objects declarations so you
can say, for example:

kdiff3 = ConfigOption('bzr.mergetool.kdiff3, 'kdiff3 %b %t %o -o %r',
                      '''How kdiff3 should be called to ..blah.''')

and bzr config bzr.mergetool will display:
kdiff3 %b %t %o -o %r

i.e. this will define the default values for config options.