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

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

    > On 11/25/2010 5:19 AM, Vincent Ladeuil wrote:
    >> Making --detect or --list mandatory sounds weird though. --list
    >> should be default but doesn't it duplicate 'bzr config
    >> bzr.mergetools' ?
    >> Or rather 'bzr config *mergetool*' to also get default_mergetool.

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

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

    >> And then... why not use bzr.mergetools.default and forbid 'default'
    >> as a valid merge tool name in your other proposal ?

    > I suppose I could. :)

    >> It would be nice to mention that --detect *also* set the mergetools
    >> in the the config (which one ?).
    >>
    >> Which one is the default in that case ? None ?

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

    > 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 ;).

    >> If that's the case, this means the user must specify which merge tool
    >> he want to use no ?

    > Correct.

    >> Since you're providing access to pre-defined merge tools as soon as
    >> you have detected them, why not go a step further and make this
    >> '--detect' step optional and provides the default command-line for,
    >> say kdiff3, if the user try to use --resolve-using kdiff3 ?

    > Yes, I could try that. With an appropriate message to the effect of
    > "auto-detecting kdiff3 on PATH"? Or is that unnecessary?

I don't think it's necessary, a mutter() (i.e. in .bzr.log) may be enough.