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

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.

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

How so?

> 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?

I really just want to get this done and merged. It's been 6 months. :P