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

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

    > On 12/7/2010 6:35 AM, Vincent Ladeuil wrote:
    >> Exactly. It occurred to me that no config option defines a set_XXX
    >> variant, and this makes even more sense if you take into account that
    >> the *user* should decide in which config file/section he wants to
    >> define them.
    >>
    >> So as I said, set_merge_tools() shouldn't be part of the Config API
    >> as it negates this feature (you can't provide a list of items to a
    >> single config object expecting it to split them across several config
    >> files, so we shouldn't even try to do that but instead provide APIs
    >> that deal with a single config option at a time). I'm also now
    >> convinced that you shouldn't provide set_default_merge_tool either,
    >> the only added value here is to check that the merge tool is valid.
    >> But again, since the config files can be updated manually, this
    >> control can be defeated and should be done by Config callers when
    >> they access the value.

    > But then how do things like qconfig update the config according to the
    > user's changes? If qconfig has to understand the structure of the config
    > file and call set_user_option('bzr.mergetools.mytool', 'blah') then
    > there's something very wrong with our design. The Config object is
    > supposed to hide those sorts of implementation details.

If the user intent is to configure a given mergetool at a given scope,
this shouldn't be hidden.

If qconfig can't provide a GUI to specify that kind of detail to specify
in which file/section the mergetool should be recorded, then a first
implementation can be restricted to update the global config only.

This could be revisited when the config infrastructure evolves.

    >> All of this should be easier if known_merge_tools was a true registry
    >> methinks.

    > How so?

By providing an access to the default values that bzr knows about until
they can be supported by the config itself.

    >> Right, I share the feeling, and I'll add that since we can't
    >> guarantee stability here we'd better: - provide a simpler mean based
    >> on get_user_option/set_user_option, - provide more advanced features
    >> (is_available) in the known_merge_tools registry, - implement a
    >> minimal set of features in qconfig (but this is outside the scope of
    >> this proposal and bzrlib ought be plugin-neutral anyway :), so let
    >> qconfig handle the list of possible merge tools and let's keep bzrlib
    >> implementation minimal. We'll provide better support when the
    >> infrastructure will be in place.

    > I still think we should keep set_merge_tools etc. on Config since that's
    > what's supposed to be hiding the messy bits of splitting config across
    > multiple locations, right?

No. As said above, the *user* should be in control, there is no way to
guess whether he wants to define the merge tool at the global level or
at the branch level or at the file level. And there is no way to guess
either how he would the merge tools to be split with an API that takes a
list...

The get accessors are enough today to *respect* what the user has
defined at any supported level, global, locations, branch.

    > I really just want to get this done and merged.

Which is why I'm trying to help you avoid bialix curses by not landing
features that we will break qbzr down the road when they need to be
updated :-p

--1F62C180DD4.1291730701/axe--