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

On Thu, November 25, 2010 10:31 am, Vincent Ladeuil wrote:
> > Making --list the default operation sounds like a good idea. Yes,
> this
> > is duplicating functionality from `bzr config`. Would you rather
> have a
> > single command called 'detect-merge-tools'?
>
> May be, if we still need it.
>
> Keep in mind that the results it produces will stick. So the resulting
> config will be wrong in the following cases:
>
> - when a new merge tool is installed by the user,
> - when an existing merged tool is updated or uninstalled by the user,
> - when a new bzr version provide new defaults for a given merge tool.

How will it be wrong? The detection process doesn't overwrite existing
merge tools. It only adds merge tools that it detects for which there
isn't already a same-named merge tool in the config.

> > I think it's nice to have a pretty-printed list of merge tools.
>
> But apart from shortening the merge tool option name, what is pretty
> here ?
>
> Morevoer it doesn't display *where* the command line is defined if the
> user put them in locations.conf or branch.conf (again, getting away from
> a core implementation means more work ;).
>
> I think the features are good to have internally, but I'm not sure we
> need to expose them at the command line level.

However, removing the mergetools command entirely and trying to
auto-detect when they use --resolve-using=kdiff3 leaves me wondering how
are users going to know that bzr already can use kdiff3/meld/etc.? I
suppose we could list the known merge tools in the merge and remerge help,
or point them to the configuration help topic where we also list the known
merge tools and the commandlines that we use for them.

This also may look a little odd/ugly in the GUI since it will have to list
all the predefined known merge tools that can be found on the PATH in
addition to the user-defined merge tools and they may have defined their
own kdiff3/meld/etc. merge tool.

I just think it's nice to have a bzr command that helps the user define
the merge tools instead of just throwing them in the deep end (aka config
file).

> > None, it doesn't set a default since the command-line doesn't need a
> > default, you always have to specify a name when you want to use a
> merge
> > tool with merge or remerge.
>
> Too bad no ?

It's very awkward, if not impossible, to have a --resolve-using option
which has an optional value to indicate the tool to use. I suppose there
could be a --resolve-using-default option or treat --resolve-using=default
as referring to the default merge tool and not a merge tool called
'default'.

> > The default only applies to qbzr and there you have qconfig to set
> > the default.
>
> plugins should be able to override the defaults provided by bzr, but if
> a user speficy it in some ways, it would be nice to respect that too
> (core implementation again ;).

I don't understand...