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

Thanks for splitting your earlier proposal, this makes it easier
to focus on each one and sorry for the delay :-}

Disclaimer: I'm certainly biased here since I'm working on
features that will make your proposal easier to implement so keep
this in mind in the following remarks.

One icon of mine is the Little Prince from Saint-Exupery
(http://en.wikiquote.org/wiki/Antoine_de_Saint-Exup%C3%A9ry)
which said unrelatedly to the Little Prince: "It seems that
perfection is attained not when there is nothing more to add, but
when there is nothing more to remove".

I'm not asking for perfection here, but I think that some parts
of your proposal may be better aligned with the existing bzrlib
features which will render your proposal easier to land.

In that spirit, I think my main objections to this proposal is
that you're forced to add features because they are not available
(yet, see
https://code.edge.launchpad.net/~vila/bzr/doc-new-config/+merge/40730)
in bzr config handling so while I'm not asking you to implement
them, I'd like the missing bits to be more obvious.

I think this is a valuable contribution but I'd like it better if
there was *less* sugar in it :)

I've tweaked your proposal at lp:~vila/bzr/mergetools/ with some
fixes for styling issues and a bit more as I was reviewing it.

I think you haven't taken into account some remarks that I agree
with too:

- quoting arguments: every single argument may contain spaces so
  internally using a splitted commandline and working on each
  should reduce the need to handle the quoting yourself (there
  are probably cases where you want to handle the quotes in the
  command line itself but I see no reason to not reuse the
  feature available in bzrlib.cmdline). All in all, I feel that
  if you consider the value of the config option as a command
  line and rely on bzrlib.cmdline to split it correctly, you
  won't have to handle so many special cases in *your* code ans
  implfiy it accordingly. This means no more *quote(arg* or
  *arg_quote* functions in bzrlib.mergetools,

- argument interpolation, using {b} instead of %b. I'd mentioned
  using longer option names too :) But as far the reference
  syntax is concerned, I think more people agree on {} rather
  than % (it's ok to have your own implementation to start with
  as you've done so far, implementing in config is my next target
  in this area (but there are other areas I'm working on too :-/)),

- more NEWS entries :) This is a user facing change and as such
  also needs to be mentioned in the what's new,

And then a couple of mine:

- why not making the tool name mandatory ? This will greatly
  simplify many parts of the code. I don't think mergetools are
  defined so often that we need to support guessing a name there
  especially when this name will be specified explicitely in all
  other cases,

- while you took care to handle errors, there are no tests to
  cover them. Please add them, test coverage for errors is
  crucial, especially when dealing with user interactions,

- many functions accept a config parameter but default to the
  global config object. While this may makes sense for testing
  purposes, I'm affraid it will tend to defeat the way we want to
  use configs (ideally the *user* can define the merge tools in
  the most convenient way and we will find it in any config
  file), so the config parameter should be mandatory. In turn,
  this means you're adding features to the config object itself,
  so until we provide better ways to do that, you should use the
  actual pattern which is to define specialized accessors there
  (see get_mail_client, get_change_editor,
  get_user_option_as_list etc). This probaly means no more
  {set|get}_merge_ functions in bzrlib.mergetools.

- the %B related stuff is unclear (I wonder if it really need to
  addressed here instead of letting the caller handle it),

Moving the accessorts in bzrlib.config may be the most important
one in my eye. There is already too many duplication in existing
plugins that were forced to enhance the config features by
re-implementing specialized config objects. So let's try to avoid
the duplication of effort here. This will also means that we can
focus on bzrlib.config when we enhance it in the future.

> + def is_available(self):
> + executable = self.get_executable()
> + return os.path.exists(executable) or _find_executable(executable)
>

Why not make the os.path.exists() call part of _find_executable() ?

Overall, this proposal is going in the right direction ;) Keep up
the good work !