Merge lp:~doxxx/bzr/mergetools into lp:bzr

Proposed by Gordon Tyler
Status: Merged
Approved by: Vincent Ladeuil
Approved revision: no longer in the source branch.
Merged at revision: 5632
Proposed branch: lp:~doxxx/bzr/mergetools
Merge into: lp:bzr
Diff against target: 603 lines (+449/-12)
12 files modified
bzrlib/config.py (+19/-0)
bzrlib/help_topics/en/configuration.txt (+34/-0)
bzrlib/mergetools.py (+116/-0)
bzrlib/osutils.py (+33/-0)
bzrlib/tests/__init__.py (+1/-0)
bzrlib/tests/features.py (+6/-11)
bzrlib/tests/test_cmdline.py (+0/-1)
bzrlib/tests/test_config.py (+39/-0)
bzrlib/tests/test_mergetools.py (+167/-0)
bzrlib/tests/test_osutils.py (+22/-0)
doc/en/release-notes/bzr-2.4.txt (+6/-0)
doc/en/whats-new/whats-new-in-2.4.txt (+6/-0)
To merge this branch: bzr merge lp:~doxxx/bzr/mergetools
Reviewer Review Type Date Requested Status
Vincent Ladeuil Approve
Andrew Bennetts Needs Fixing
John A Meinel Pending
Martin Packman Pending
Review via email: mp+40923@code.launchpad.net

This proposal supersedes a proposal from 2010-10-17.

Commit message

Added external merge tool management module, bzrlib.mergetools.

Description of the change

External merge tool support in bzr and related GUI tools is not very well defined and there are currently two incompatible mechanisms for setting an external merge tool.

This merge proposal seeks to improve this by defining a bzrlib.mergetools module which provides functions for checking availability and invoking a merge tool command line. Methods have also been added to bzrlib.config.Config to retrieve the list of user-defined merge tools, find a merge tool by name (falling back to a list of predefined merge tools in the mergetools module if the name cannot be found in the config).

Related branches:

lp:~doxxx/bzr/mergetools-commands adds a '--resolve-using' option to 'bzr merge' and 'bzr remerge' and '--using' option to 'bzr resolve' to invoke an external merge tool for text conflicts.

lp:~qbzr-dev/qbzr/mergetools changes qconfig and qconflicts to make use of bzrlib.mergetools and its associated config methods.

To post a comment you must log in.
Revision history for this message
Gordon Tyler (doxxx) wrote : Posted in a previous version of this proposal

Could I get some feedback on this? Right approach? Too limited in scope?

Revision history for this message
John C Barstow (jbowtie) wrote : Posted in a previous version of this proposal

It looks like a big step in the right direction, and enough of an improvement to be worth merging from my perspective. There are two additional things I'd really like to see, but IMO they could be handled as issues in their own right.

1) I would like to see 'bzr diff' handled in the same way, so there is a consistent approach for external tools.

2) I think that the 'add' operation should allow for optionally specifying a name. This covers the ability to add the same tool twice with different flags, the ability to work with two executables that happen to have the same name, and the ability to choose an alias that conveys additional information (such as 'cygwin-sdiff' on a Windows platform).

If we choose to merge this, I think the user and admin guides will need to be updated, with an eye to explaining both how to use the functionality and how to determine when it is most appropriate.

Revision history for this message
Martin Packman (gz) wrote : Posted in a previous version of this proposal

I don't know about any of the high level design stuff, but noticed a few things reading through the diff that I was getting round to writing up.

I like that you're using cmdline here, but why are you then getting in the business of quoting the arguments again? If there's a reason not to pass subprocess.call a list, that should be documented. Also, you're using shell=True always, as that's creating an intermediate process you should say why you need it.

Using percent-followed-by-letter is slightly misleading for the templating, as this is a little more magical than straight interpolation. Also odd that I could use %TMP% in my line and be suprised by just %T expanding. I'm guessing there are good historical reasons though.

What happens with non-ascii filenames? I see no handling, so will you just be throwing UnicodeError somewhere?

+def _resolve_using_merge_tool(tool_name, conflicts):

I'm not mad about this function or the --resolve-using interface, but don't have any great alternatives.

+ If you need to include options in your external merge tool's
+ command-line, insert '--' before the command-line to prevent bzr from
+ processing them as options to the ``bzr mergetools`` command:

This seems silly. Why is:

+ bzr mergetools --add -- kdiff3 %b %t %o -o %r

Preferable to `bzr mergetools --add "kdiff3 %b %t %o -o %r"`?

+ def get_name(self):
+ name = os.path.basename(self.get_executable())
+ if sys.platform == 'win32' and name.lower().endswith('.exe'):
+ name = name[:-4]

What's special about .exe over .bat or .com? Should this be using os.path.splitext and the PATHEXT envvar?

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

Don't like this look-before-you-leap style. Would prefer you catch the right OSError from subprocess.call to detect and warn about missing executables.

+# Authors: Robert Collins <email address hidden>

You conned Robert into writing your tests for you? :)

review: Abstain
Revision history for this message
Gordon Tyler (doxxx) wrote : Posted in a previous version of this proposal
Download full text (4.4 KiB)

Thanks John and Martin for the feedback!

> 1) I would like to see 'bzr diff' handled in the same way, so there is a
> consistent approach for external tools.

I have been giving this some thought but I wasn't sure if it would be trying to do too much in one patch.

> 2) I think that the 'add' operation should allow for optionally specifying a
> name. This covers the ability to add the same tool twice with different flags,
> the ability to work with two executables that happen to have the same name,
> and the ability to choose an alias that conveys additional information (such
> as 'cygwin-sdiff' on a Windows platform).

I have thought about this as well. It complicates the storage in the config, although not to the point of being infeasible. And as you say, it's almost certainly going to be necessary in some situations. I'll see what I can do.

> I like that you're using cmdline here, but why are you then getting in the
> business of quoting the arguments again? If there's a reason not to pass
> subprocess.call a list, that should be documented. Also, you're using
> shell=True always, as that's creating an intermediate process you should say
> why you need it.

I am passing a list to subprocess.call. The commandline is given by the user and stored as a single string and I use cmdline.split to convert it to a list before calling subprocess.call. Therefore, when performing filename substitution, I have to quote the filenames so that cmdline.split will split it correctly. However, I probably could do the cmdline.split first and then do filename substitution in each argument without worrying about quoting. I'll try that.

I don't recall why I was using shell=True. It must have seemed like a good idea at the time. I'll remove it.

> Using percent-followed-by-letter is slightly misleading for the templating, as
> this is a little more magical than straight interpolation. Also odd that I
> could use %TMP% in my line and be suprised by just %T expanding. I'm guessing
> there are good historical reasons though.

It's the same syntax as the old external merge tool support as well as the extmerge plugin. Python string interpolation syntax may be too arcane/verbose for users. Although, I suppose they are probably going to be developers. Still, "kdiff3 %(base)s %(this)s %(other)s -o %(result)s" is kinda icky.

> What happens with non-ascii filenames? I see no handling, so will you just be
> throwing UnicodeError somewhere?

Good point, I should be using unicode literals.

> +def _resolve_using_merge_tool(tool_name, conflicts):
>
> I'm not mad about this function or the --resolve-using interface, but don't
> have any great alternatives.

Before this, I had a separate command for invoking a merge tool on conflicts but a quick survey on the bzr list indicated this wasn't what people were expecting, and they suggested something like the above.

> + If you need to include options in your external merge tool's
> + command-line, insert '--' before the command-line to prevent bzr from
> + processing them as options to the ``bzr mergetools`` command:
>
> This seems silly. Why is:
>
> + bzr mergetools --add -- kdiff3 %b %t %o -o ...

Read more...

Revision history for this message
Martin Pool (mbp) wrote : Posted in a previous version of this proposal
Download full text (34.1 KiB)

Sorry for the delay. This looks pretty nice. And thanks too for a
very clear cover letter.

It should get a NEWS entry, a whatsnew entry and some description in
the user guide. (whatsnew could be very similar to your cover
letter.) Experience shows that if we don't document new features when
they are added, it is harder to come back and do them later.

Strictly speaking it seems this is redundant with editing the config
file, or with vila's 'config' command, but perhaps the configuration
here is complex enough that it's worth having a special option.

New Pythons have a "hello {name}" syntax
<http://docs.python.org/library/string.html#format-string-syntax> that
would be ideal for this: less error-prone than "%(name)s" but more
explicit and extensible than "%n". Unfortunately that is only in 2.6
and later, but perhaps we could simulate it on old Pythons...?

On 17 October 2010 13:48, Gordon Tyler <email address hidden> wrote:
> Gordon Tyler has proposed merging lp:~doxxx/bzr/mergetools into lp:bzr.
>
> Requested reviews:
>  bzr-core (bzr-core)
> Related bugs:
>  #489915 [master] external diff/merge configuration needs serious rework
>  https://bugs.launchpad.net/bugs/489915
>
>
> External merge tool support in bzr and related GUI tools is not very well defined and there are currently two incompatible mechanisms for setting an external merge tool.
>
> This merge proposal seeks to improve this by defining a bzrlib.mergetools module which provides an API for managing a list of external merge tools, adding a 'bzr mergetools' command which allows the user to manage the list of mergetools, and adding a '--resolve-using' option to 'bzr merge' and 'bzr remerge' to invoke an external merge tool for text conflicts.
>
> There will be a corresponding merge proposal for qbzr to make use of 'bzrlib.mergetools' in qconfig and qconflicts.
>
> Merge tools are defined by their command-line. The name of a merge tool, for the purposes of display and reference in the 'bzr merge' and 'bzr remerge' commands, is derived from the basename of the executable, stripping '.exe' if on win32. In the future, this could be separated out into a distinct field to allow multiple definitions of a merge tool using the same executable but different arguments, which may be necessary for jbowtie's binary merge idea.
>
> The mergetools module also tracks a default merge tool that will be used by qconfig and qconflicts. The 'bzr mergetools' command doesn't provide a method for setting it since it is not necessary for any of the bzr commands that make use of external merge tools.
>
> The 'bzr mergetools' command provides the following operations:
>
> - add: Adds a new external merge tool
> - update: Updates an existing merge tool by name
> - remove: Removes an existing merge tool by name
> - detect: Automatically detects known merge tools on the PATH
> - list: Lists the external merge tools that have been defined
>
> --
> https://code.launchpad.net/~doxxx/bzr/mergetools/+merge/38663
> Your team bzr-core is requested to review the proposed merge of lp:~doxxx/bzr/mergetools into lp:bzr.
>
> === modified file 'bzrlib/builtins.py'
> --- bzrlib/builtins.py  2010-10-15 11:30:54 +0000
> +++ b...

Revision history for this message
Alexander Belchenko (bialix) wrote : Posted in a previous version of this proposal

Martin Pool пишет:
> New Pythons have a "hello {name}" syntax
> <http://docs.python.org/library/string.html#format-string-syntax> that
> would be ideal for this: less error-prone than "%(name)s" but more
> explicit and extensible than "%n". Unfortunately that is only in 2.6
> and later, but perhaps we could simulate it on old Pythons...?

Martin, actually bzr uses exactly the same syntax in version-info
command. It should not be too hard to re-use that template engine,
right? ;-)

Revision history for this message
Gordon Tyler (doxxx) wrote : Posted in a previous version of this proposal

> It should get a NEWS entry, a whatsnew entry and some description in
> the user guide. (whatsnew could be very similar to your cover
> letter.) Experience shows that if we don't document new features when
> they are added, it is harder to come back and do them later.

I've already added entries to the release notes, which seem to be the intended replacement for NEWS. I'll try find the whatsnew doc and update it. I'll also try my hand ad updating the user guide, but a doc-writer I am not, so it will most likely require the ministrations of Neil M-B.

> Strictly speaking it seems this is redundant with editing the config
> file, or with vila's 'config' command, but perhaps the configuration
> here is complex enough that it's worth having a special option.

Once I add support for naming the merge tools, the resultant config structure may be a bit too complex for newbies to edit. While newbies would most likely be using qbzr and thus qconfig, I would still like to make it possible to manage the tools via the command-line.

> New Pythons have a "hello {name}" syntax
> <http://docs.python.org/library/string.html#format-string-syntax> that
> would be ideal for this: less error-prone than "%(name)s" but more
> explicit and extensible than "%n". Unfortunately that is only in 2.6
> and later, but perhaps we could simulate it on old Pythons...?

Should easy enough to emulate, especially since we'll only support very basic substitution.

Revision history for this message
Gordon Tyler (doxxx) wrote : Posted in a previous version of this proposal

Any advice on how to make this Unicode-safe? I've got some local changes here that use Unicode literals for everything that is combined with the user-provided command-line. Is that all I need or is there something more I need to do?

Also, in a blackbox test, how would I validate that my 'bzr mergetools' command is modifying the bzr config correctly, without actually affecting the user's real bzr config?

Revision history for this message
Gordon Tyler (doxxx) wrote : Posted in a previous version of this proposal

I've figured out how to do the blackbox tests with spiv's help.

Revision history for this message
Martin Pool (mbp) wrote : Posted in a previous version of this proposal

On 26 October 2010 21:56, Gordon Tyler <email address hidden> wrote:
> Any advice on how to make this Unicode-safe? I've got some local changes here that use Unicode literals for everything that is combined with the user-provided command-line. Is that all I need or is there something more I need to do?

Can you be more specific?

> Also, in a blackbox test, how would I validate that my 'bzr mergetools' command is modifying the bzr config correctly, without actually affecting the user's real bzr config?

If you subclass TestCaseinTempDir you should automatically get a
temporary BZR_HOME and config framework.

--
Martin

Revision history for this message
Gordon Tyler (doxxx) wrote : Posted in a previous version of this proposal
Download full text (7.8 KiB)

Martin (gz) asked "What happens with non-ascii filenames? I see no handling, so will you just be throwing UnicodeError somewhere?"

I'm not sure what handling I'm meant to do other than use unicode literals (e.g. u'blah') when working with the command-line in a merge tool, like so:

=== modified file 'bzrlib/mergetools.py'
--- a/bzrlib/mergetools.py 2010-10-27 01:56:55 +0000
+++ b/bzrlib/mergetools.py 2010-10-27 04:35:10 +0000
@@ -39,11 +39,11 @@

 substitution_help = {
- '%b' : 'file.BASE',
- '%t' : 'file.THIS',
- '%o' : 'file.OTHER',
- '%r' : 'file (output)',
- '%T' : 'file.THIS (temp copy, used to overwrite "file" if merge succeeds)'
+ u'%b' : u'file.BASE',
+ u'%t' : u'file.THIS',
+ u'%o' : u'file.OTHER',
+ u'%r' : u'file (output)',
+ u'%T' : u'file.THIS (temp copy, used to overwrite "file" if merge succeeds)
'
 }

@@ -53,7 +53,8 @@
     return retcode

-_WIN32_PATH_EXT = [ext.lower() for ext in os.getenv('PATHEXT', '').split(';')]
+_WIN32_PATH_EXT = [unicode(ext.lower())
+ for ext in os.getenv('PATHEXT', '').split(';')]

 class MergeTool(object):
@@ -80,20 +81,20 @@
         return name

     def get_commandline(self):
- return ' '.join(self._commandline)
+ return u' '.join(self._commandline)

     def get_commandline_as_list(self):
         return self._commandline

     def get_executable(self):
         if len(self._commandline) < 1:
- return ''
+ return u''
         return self._commandline[0]

     def get_arguments(self):
         if len(self._commandline) < 2:
- return ''
- return ' '.join(self._commandline[1:])
+ return u''
+ return u' '.join(self._commandline[1:])

     def set_executable(self, executable):
         self._commandline[:1] = [executable]
@@ -130,26 +131,26 @@
         tmp_file = None
         subst_args = []
         for arg in args:
- arg = arg.replace('%b', filename + '.BASE')
- arg = arg.replace('%t', filename + '.THIS')
- arg = arg.replace('%o', filename +'.OTHER')
- arg = arg.replace('%r', filename)
- if '%T' in arg:
- tmp_file = tempfile.mktemp("_bzr_mergetools_%s.THIS" %
+ arg = arg.replace(u'%b', filename + u'.BASE')
+ arg = arg.replace(u'%t', filename + u'.THIS')
+ arg = arg.replace(u'%o', filename + u'.OTHER')
+ arg = arg.replace(u'%r', filename)
+ if u'%T' in arg:
+ tmp_file = tempfile.mktemp(u"_bzr_mergetools_%s.THIS" %
                                            os.path.basename(filename))
- shutil.copy(filename + ".THIS", tmp_file)
- arg = arg.replace('%T', tmp_file)
+ shutil.copy(filename + u".THIS", tmp_file)
+ arg = arg.replace(u'%T', tmp_file)
             subst_args.append(arg)
         return subst_args, tmp_file

 _KNOWN_MERGE_TOOLS = (
- 'bcompare %t %o %b %r',
- 'kdiff3 %b %t %o -o %r',
- 'xxdiff -m -O -M %r %t %b %o',
- 'meld %b %T %o',
- 'opendiff %t %o -ancestor %b -merge %r',
- 'winmergeu %r',
+ u'bcompare %t %o %b %r',
+ u'kdiff3 %b %...

Read more...

Revision history for this message
Martin Pool (mbp) wrote : Posted in a previous version of this proposal

On 27 October 2010 00:41, Gordon Tyler <email address hidden> wrote:
> Martin (gz) asked "What happens with non-ascii filenames? I see no handling, so will you just be throwing UnicodeError somewhere?"

I think that particular diff you attached seems reasonable -
human-readable strings that aren't part of binary file formats are
most naturally in unicode.

The best way to handle this would be just to add some tests that use
non-ascii filenames. (They'll need to depend on the unicode
filesystem feature.)

--
Martin

Revision history for this message
Martin Packman (gz) wrote : Posted in a previous version of this proposal

> I'm not sure what handling I'm meant to do other than use unicode literals
> (e.g. u'blah') when working with the command-line in a merge tool, like so:

I'd suggest step one should be testing, so you know what is breaking where. Just using unicode literals won't save you because of python limitations:

<http://bugs.python.org/issue1759845>

> I am passing a list to subprocess.call. The commandline is given by the user
> and stored as a single string and I use cmdline.split to convert it to a list
> before calling subprocess.call. Therefore, when performing filename
> substitution, I have to quote the filenames so that cmdline.split will split
> it correctly. However, I probably could do the cmdline.split first and then do
> filename substitution in each argument without worrying about quoting. I'll
> try that.

I see _optional_quote_arg is still being used. I think you should be storing a list not a string in self._commandline so you can delete that and the related functions.

> Because the quoting, if necessary, could get extremely funky. I figured if
> they could just write exactly like they normally would, then it would be less
> confusing.

We don't want the user to be doing any quoting. Needing to wrap every %var in quotes just in case it contains a space is shell-level stupidity we're avoiding by handling that for them.

> This is used in the GUI to indicate that the selected tool is not available
> and to disable the button used to launch the tool.

I really don't like grovelling through the filesystem with _find_executable as an idea. (Does it even have a clear licence?) The 'detect' command for merge tools does seem reasonable though so maybe it's unavoidable.

Revision history for this message
Gordon Tyler (doxxx) wrote : Posted in a previous version of this proposal

> I see _optional_quote_arg is still being used. I think you should be storing a
> list not a string in self._commandline so you can delete that and the related
> functions.

I pushed new revisions almost a day ago which did exactly that.

> We don't want the user to be doing any quoting. Needing to wrap every %var in
> quotes just in case it contains a space is shell-level stupidity we're
> avoiding by handling that for them.

I meant quoting which may be necessary for passing arguments correctly to the merge tool they're using. I agree that the user should not have to quote the substitution markers in case they're used with files containing spaces. That's part of the problem with the current system.

> I really don't like grovelling through the filesystem with _find_executable as
> an idea. (Does it even have a clear licence?) The 'detect' command for merge
> tools does seem reasonable though so maybe it's unavoidable.

That find_executable function is exactly as it appears on the webpage I reference in the comment above it. There is no license that I can see. Since it was on a site that explicitly accepts and re-publishes "snippets" of code, I assumed that it was basically in the public domain.

I really like the detect command because it takes a lot of the work out of configuring the merge tools, which is one of the primary goals I'm trying to accomplish here.

Revision history for this message
Gordon Tyler (doxxx) wrote : Posted in a previous version of this proposal

I've made merge tool names user-configurable as John Barstow suggested. Unfortunately, I couldn't make the merge tool name optional in the `bzr mergetools --add` command since I don't know how to make an option that optionally takes a value.

Revision history for this message
Gordon Tyler (doxxx) wrote : Posted in a previous version of this proposal

The way the merge tools are stored in the bzr config has changed now. Instead of just a list of command-lines, it's now one option which contains a list of tool names and an option named for each tool which contains the command-line. Additionally, the command-lines are stored in list form to preserve spaces within arguments. This does make it a little less user-editable. I could change it back to storing as a simple string but then I'd have to check and quote arguments that contain spaces.

Any comments?

Revision history for this message
Martin Packman (gz) wrote : Posted in a previous version of this proposal

> I pushed new revisions almost a day ago which did exactly that.

Ha, the problem with writing up some comments one evening and posting them the next, you're way too fast. :)

The new unicode test looks like a good start, and sorry, should have mentioned failures on tests like that tend to break things badly currently, you'll want testtools 0.9.5 or later and either <lp:~gz/bzr/escape_selftest_console_output_633216> or a utf-8 console. However, there does need to be a test that actually launches some kind of real subprocess, as that's where non-ascii filenames are most likely to break down. Don't need a real merge tool, echo on the shell would probably do.

I agree storing lists in the config file hurts editability. Slightly annoying as to get back to a string does mean quoting even though templated command lines are unlikely to ever need it. On windows you can use subprocess.list2cmdline but you might want your quoting function back for nix even though `" ".join(map(repr, commandline))` should basically work.

Revision history for this message
Martin Pool (mbp) wrote : Posted in a previous version of this proposal

On 27 October 2010 23:05, Gordon Tyler <email address hidden> wrote:
> I've made merge tool names user-configurable as John Barstow suggested. Unfortunately, I couldn't make the merge tool name optional in the `bzr mergetools --add` command since I don't know how to make an option that optionally takes a value.

Our ui style says we don't have optional option values; it makes the
syntax a bit confusing.

You could have a second option to specify the tool name?

--
Martin

Revision history for this message
Gordon Tyler (doxxx) wrote : Posted in a previous version of this proposal

> > I've made merge tool names user-configurable as John Barstow suggested.
> Unfortunately, I couldn't make the merge tool name optional in the `bzr
> mergetools --add` command since I don't know how to make an option that
> optionally takes a value.
>
> Our ui style says we don't have optional option values; it makes the
> syntax a bit confusing.
>
> You could have a second option to specify the tool name?

I've been thinking about this and I think it would be confusing to have --add behave differently --update and --remove. I'll keep it as it is, requiring a name to be given.

Revision history for this message
Gordon Tyler (doxxx) wrote : Posted in a previous version of this proposal

I tried writing Unicode blackbox tests but testtools and bzr's selftest is quite broken for tests that output Unicode or UTF-8, so I've shelved that for later. Any idea when this is going get fixed?

Revision history for this message
Gordon Tyler (doxxx) wrote : Posted in a previous version of this proposal

I changed the storage format back to strings in the config file, but this time making sure they're quoted as necessary.

Revision history for this message
Martin Packman (gz) wrote : Posted in a previous version of this proposal

Current revision covers all of my queries bar the non-ascii one. See my last comment for answers on your testing problems Gordon, the fix should hopefully land shortly once PQM gets an updated testtools package.

Branch still wants a high level review by someone else.

Revision history for this message
Gordon Tyler (doxxx) wrote : Posted in a previous version of this proposal

Thanks, Martin. Will notification of the update to PQM and bzr be psoted to the bzr list?

Revision history for this message
John A Meinel (jameinel) wrote : Posted in a previous version of this proposal
Download full text (5.9 KiB)

I think the idea of having a common mergetools set is a good one. And is
something that we want to bring into core. I think this still needs some
updates, but it is getting close to a point where I would be happy to merge it.

18 + if merge_tool is None:
19 + available = '\n '.join([mt.get_name() for mt in
20 + mergetools.get_merge_tools()
21 + if mt.is_available()])
22 + raise errors.BzrCommandError('Unrecognized merge tool: %s\n\n'
23 + 'Available merge tools:\n'
24 + ' %s' % (tool_name, available))

^- This specific code seems to be something that should live in mergetools itself. I generally don't like top-level functions in 'builtins.py'. Maybe just move the whole thing to mergetools and use "mergetools.resolve_using_mergetool()" inside the command code?

Importing "mergetools" should also be done in the "lazy_import" section. which you may have already done.

63 + retval = self._do_merge(merger, change_reporter, allow_pending,
64 + verified)
65 + if retval != 0 and resolve_using is not None:
66 + _resolve_using_merge_tool(resolve_using, tree.conflicts())
67 + return retval

^- The spacing is odd here (yes, I realize Copy & Paste messed it up). But what I see is about 8 character indentation, when I would expect 4. Maybe you have a tab char? (sources with tab characters will be rejected by the test suite.)

104 ('cmd_resolve', ['resolved'], 'bzrlib.conflicts'),
105 ('cmd_conflicts', [], 'bzrlib.conflicts'),
106 ('cmd_sign_my_commits', [], 'bzrlib.sign_my_commits'),
107 +<<<<<<< TREE
108 ('cmd_test_script', [], 'bzrlib.cmd_test_script'),
109 +=======
110 + ('cmd_test_script', [], 'bzrlib.tests.script'),
111 + ('cmd_mergetools', [], 'bzrlib.mergetools'),
112 +>>>>>>> MERGE-SOURCE

^- most lists like this we try to keep in sorted order. I realize this one isn't, but it probably should be. (That way, most inserts go into a different location, rather than all-at-the-end and it helps avoid spurious conflicts like this.)

There is duplicate code between "MergeTool.__init__" and "MergeTool.set_commandline". In general, it seems like there are a lot of functions in there that we won't actually want to use. So potentially paring it down a bit would be a good thing. However, if we do want it, then __init__ should probably just call self.set_commandline() rather than copying the code locally.

I agree that "cmd_mergetools" seems a bit redundant with "cmd_config", but it also has stuff like --detect, which means it can do more than config would.

It is probably fine to leave it as is.

716 === added file 'bzrlib/tests/test_mergetools.py'
717 --- bzrlib/tests/test_mergetools.py 1970-01-01 00:00:00 +0000
718 +++ bzrlib/tests/test_mergetools.py 2010-10-30 22:41:24 +0000
719 @@ -0,0 +1,242 @@
720 +# -*- coding: utf-8 -*-
We don't use coding statements. if you need non-ascii characters, use their escaped form. (u'\xb5', etc.)

The tests are a bit clustered (and some lines are longer than 80 chars). Rather than doing:

749 +class TestMergeTool(tests.TestCaseInTempDir):
750 + def test_basics(self):
751 + mt = mergetools.MergeTool('tool', '/path/to/tool --opt %b -x %t %o --stuff %r')
752 + self.assertEquals('/path/to/tool --opt %b -x %t %o --stuff %...

Read more...

review: Needs Fixing
Revision history for this message
Gordon Tyler (doxxx) wrote : Posted in a previous version of this proposal

Thanks for the review, John. That all seems quite reasonable. I'll get right on it!

Revision history for this message
Gordon Tyler (doxxx) wrote : Posted in a previous version of this proposal

John, I think I've taken care of all the things you mentioned. I removed the get_arguments and set_arguments methods on MergeTool since, as you said, they weren't being used and they're somewhat redundant. The rest of the getter/setter methods are being used in one way or another, or are intended for use by qbzr/etc.

I also used u"\u1234" syntax for the unicode tests since I wasn't exactly sure the u"\xb5" syntax was right.

Revision history for this message
Gary van der Merwe (garyvdm) wrote : Posted in a previous version of this proposal

I would personally like a %B - 'file.BASE (temp copy, used to overwrite "file" if merge succeeds)' option, so that I can do meld %t %B %o.

I'm setting this mp back to "Needs Review."

Revision history for this message
Vincent Ladeuil (vila) wrote : Posted in a previous version of this proposal

Wow, nice work and numerous reviews already, but still, this has not landed...

This is certainly related to the size of the patch so splitting it into several parts will help, but since your have already invested quite a bunch of time on it, I suspect you won't be rejoiced by such a proposal :-/

Anyway, I have some high level remarks:

- if you want to split it anyway (who knows ;) the configuration part and the command behaviour modifications sounds like good candidates,

- you use a FakeConfig for some tests which is a bit risky, especially considering that you didn't take the {option} syntax into account as suggested by poolie (and I agree this would be better than using the obscure %B %t %o options). I think that configuring such tools is rarely done and will most of the time be done by copying an existing one. In this context having verbose options means you don't have to search the documentation for the meaning of the short ones. I won't block on that though as I'd like this syntax to be supported by bzrlib.config and that's outside the scope of your proposal.

- like John, I think there is some overlapping with the ``bzr config`` command and that will be a maintenance burden down the road, mergetools can keep the list and detect subcommands, also note that checking the external merge tool availability at this point won't fly, you are forbidding users to configure them under special setups and you still need to check their availability when you *use* them. Also, by using special commands to add/modify/delete there you also forbid setting such tools in more specific ways in locations.conf and branch.conf (which I want to allow for *any* option in the new config scheme),

- you said that the merge tool is called for text conflicts but I see no tests that you don't try to call it for other types of conflicts,

- bzrlib.mergetools.py use DOS line endings, please use Unix line endings (and watch for line with only spaces :-/),

- regarding docs, most of the cmd_mergetools docstring should be part of configuration doc, that's where people are directed for questions about config options,

- _KOWN_MERGE_TOOLS really want to be a registry :)

I hope I'm sounding too negative here, I really think this proposal is valuable and I will do my best to help you land it.

review: Needs Fixing
Revision history for this message
Gordon Tyler (doxxx) wrote : Posted in a previous version of this proposal
Download full text (3.9 KiB)

Thanks for the review, Vincent. :)

On Thu, November 11, 2010 11:32 am, Vincent Ladeuil wrote:
> This is certainly related to the size of the patch so splitting it into
> several parts will help, but since your have already invested quite a
> bunch of time on it, I suspect you won't be rejoiced by such a proposal
> :-/

I'm not sure how I would actually go about splitting it. Branch from
lp:~doxxx/bzr/mergetools and then remove the extra bits from
lp:~doxxx/bzr/mergetools?

> - if you want to split it anyway (who knows ;) the configuration part and
> the command behaviour modifications sounds like good candidates,

Agreed.

> - you use a FakeConfig for some tests which is a bit risky, especially
> considering that you didn't take the {option} syntax into account as
> suggested by poolie (and I agree this would be better than using the
> obscure %B %t %o options). I think that configuring such tools is rarely
> done and will most of the time be done by copying an existing one. In this
> context having verbose options means you don't have to search the
> documentation for the meaning of the short ones. I won't block on that
> though as I'd like this syntax to be supported by bzrlib.config and that's
> outside the scope of your proposal.

I think I wrote those tests before I discovered that TestCaseInTempDir
would isolate tests from changing the user's real config. I'll try
changing the tests to use a real config object.

Regarding verbose placeholders, I'm beginning to agree with you and other
reviewers. So, I can do the substitution myself for now and then once the
config object is capable of doing it, we can change mergetools to use
that?

> - like John, I think there is some overlapping with the ``bzr config``
> command and that will be a maintenance burden down the road, mergetools

The problem I have with using ``bzr config`` is that it requires that the
user understand the layout of the config file, that they must add the name
to one setting and then create another setting using that name which
contains the commandline. They need to get the quoting right and the
commas right, etc. This is not just some simple "name=value" setting like
email. Why can't we have both?

> can keep the list and detect subcommands, also note that checking the
> external merge tool availability at this point won't fly, you are
> forbidding users to configure them under special setups and you still need
> to check their availability when you *use* them. Also, by using special

Checking of availability only logs a warning if it's not available. I
think its useful because it warns them immediately so that they're not
surprised later when they're trying do resolve conflicts and they have to
stop what they're doing to go fix the configuration.

> commands to add/modify/delete there you also forbid setting such tools in
> more specific ways in locations.conf and branch.conf (which I want to
> allow for *any* option in the new config scheme),

That's a good point. I don't really have a good solution for that.

> - you said that the merge tool is called for text conflicts but I see no
> tests that you don't try to call it for other types of conflicts,

Not sure how to...

Read more...

Revision history for this message
Vincent Ladeuil (vila) wrote : Posted in a previous version of this proposal
Download full text (6.0 KiB)

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

    > Thanks for the review, Vincent. :)
    > On Thu, November 11, 2010 11:32 am, Vincent Ladeuil wrote:
    >> This is certainly related to the size of the patch so splitting it into
    >> several parts will help, but since your have already invested quite a
    >> bunch of time on it, I suspect you won't be rejoiced by such a proposal
    >> :-/

    > I'm not sure how I would actually go about splitting it. Branch from
    > lp:~doxxx/bzr/mergetools and then remove the extra bits from
    > lp:~doxxx/bzr/mergetools?

Whatever works :) The above is ok. Then you'll have to decide whether
you want to regularly merge from one branch to the other, use a pipeline
or a loom. And don't forget to set the pre-requisite branch when submitting.

    >> - if you want to split it anyway (who knows ;) the configuration part and
    >> the command behaviour modifications sounds like good candidates,

    > Agreed.

Great.

    >> - you use a FakeConfig for some tests which is a bit risky, especially
    >> considering that you didn't take the {option} syntax into account as
    >> suggested by poolie (and I agree this would be better than using the
    >> obscure %B %t %o options). I think that configuring such tools is rarely
    >> done and will most of the time be done by copying an existing one. In this
    >> context having verbose options means you don't have to search the
    >> documentation for the meaning of the short ones. I won't block on that
    >> though as I'd like this syntax to be supported by bzrlib.config and that's
    >> outside the scope of your proposal.

    > I think I wrote those tests before I discovered that TestCaseInTempDir
    > would isolate tests from changing the user's real config. I'll try
    > changing the tests to use a real config object.

Ok.

    > Regarding verbose placeholders, I'm beginning to agree with you
    > and other reviewers. So, I can do the substitution myself for now
    > and then once the config object is capable of doing it, we can
    > change mergetools to use that?

Hehe, chiken-and-egg problem :)

All things being equal, I think implementing it directly into config may
be require less work overall, but let's not introduce more dependencies
there, postpone this precise point and we'll see what is available when
you come to it (configobj provides the necessary hooks and most of the
infrastructure, including the common errors (definition loops, undefined
option) so no need to re-implement from scratch). I still need to submit
my braindump on the new config scheme I envision where I've already
describe some related stuff. I should clean it up and submit it
tomorrow.

    >> - like John, I think there is some overlapping with the ``bzr config``
    >> command and that will be a maintenance burden down the road, mergetools

    > The problem I have with using ``bzr config`` is that it requires that the
    > user understand the layout of the config file, that they must add the name
    > to one setting and then create another setting using that name which
    > contains the commandline. They need to get the quoting right and the
    > commas right...

Read more...

Revision history for this message
Gary van der Merwe (garyvdm) wrote : Posted in a previous version of this proposal

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/11/2010 20:19, Vincent Ladeuil wrote:
> How about using option names like 'bzr.mergetool.bcompare'
> 'bzr.mergetool.kdiff3' and so on ? Then we could query the config object
> for option whose names start with 'bzr.mergetool'. Would this address
> some of your concerns ?

I was thinking it may be better to have the config formated like this:

[DEFAULT]
default_mergetool = meld

[MERGETOOLS]
meld = /usr/bin/meld %t %r %o
kdiff3 = ....

Similar to [ALIASES], bzr-bookmark's [BOOKMARKS], qbzr's [EXTDIFF]. But
unfortunately bzr config does not provide access to view/change these.
It would nice if it could.

Gary

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkzcO8QACgkQd/3EdwGKOh0mvQCcD7G/Rjk9TK9Q4NNs/2wtazLx
H94AnA2djx2oIhd1+fnxUlf3wB7mm5sZ
=oRR8
-----END PGP SIGNATURE-----

Revision history for this message
Gordon Tyler (doxxx) wrote : Posted in a previous version of this proposal

On Thu, November 11, 2010 1:19 pm, Vincent Ladeuil wrote:
> > I'm not sure how I would actually go about splitting it. Branch from
> > lp:~doxxx/bzr/mergetools and then remove the extra bits from
> > lp:~doxxx/bzr/mergetools?
>
> Whatever works :) The above is ok. Then you'll have to decide whether
> you want to regularly merge from one branch to the other, use a pipeline
> or a loom. And don't forget to set the pre-requisite branch when
> submitting.

Won't the removal of extra bits in the original get merged into the second
when I try and merge new changes in the original into the second?

> All things being equal, I think implementing it directly into config may
> be require less work overall, but let's not introduce more dependencies
> there, postpone this precise point and we'll see what is available when
> you come to it (configobj provides the necessary hooks and most of the
> infrastructure, including the common errors (definition loops, undefined
> option) so no need to re-implement from scratch). I still need to submit
> my braindump on the new config scheme I envision where I've already
> describe some related stuff. I should clean it up and submit it
> tomorrow.

Okay, leaving it as is then.

> How about using option names like 'bzr.mergetool.bcompare'
> 'bzr.mergetool.kdiff3' and so on ? Then we could query the config object
> for option whose names start with 'bzr.mergetool'. Would this address
> some of your concerns ?

I already create option names like that but I didn't see a way to query
for options whose names start with a prefix, thus the separate list
option. I'll take another look at the config stuff to see how to do this.

But yeah, if I can get this working then it does become more
user-editable, in which case I would be okay with removing the --add,
--update and --remove options from the mergetools command.

Should these options go into a different section in the config file? Or
would that prevent putting them in locations.conf and branch.conf?

What about tool names which have "funny" characters in them like spaces or
other stuff? What's legal in an option name?

> isinstance(conflict, conflicts.TextConflict) to filter only text
> conflicts, bt.test_conflicts, bb.test_conflicts should contain almost
> all the tests related to conflicts otherwise.

Thanks.

> Right, my editor does to, but if we can keep a coherent line ending in
> the code base, that would be better.

Will do.

> Well, two places for the same doc mean they should be both maintained,
> that's error-prone. All other config options are described
> bzrlib/help_topics/en/configuration.txt, it makes our life easier when
> we want to point users to this place.

Okay.

> >> - _KOWN_MERGE_TOOLS really want to be a registry :)
>
> > I'll have a look at that.
>
> May be not :) If you rely on a naming rule for that...

It's just a place to list the names and command-lines of merge tools that
we already know about, so that we can try detect them on the path. Is a
registry suitable for that?

Revision history for this message
Vincent Ladeuil (vila) wrote : Posted in a previous version of this proposal

>>>>> Gary van der Merwe <email address hidden> writes:

    > On 11/11/2010 20:19, Vincent Ladeuil wrote:
    >> How about using option names like 'bzr.mergetool.bcompare'
    >> 'bzr.mergetool.kdiff3' and so on ? Then we could query the config object
    >> for option whose names start with 'bzr.mergetool'. Would this address
    >> some of your concerns ?

    > I was thinking it may be better to have the config formated like this:

    > [DEFAULT]
    > default_mergetool = meld

    > [MERGETOOLS]
    > meld = /usr/bin/meld %t %r %o
    > kdiff3 = ....

    > Similar to [ALIASES], bzr-bookmark's [BOOKMARKS], qbzr's [EXTDIFF]. But
    > unfortunately bzr config does not provide access to view/change these.
    > It would nice if it could.

bzr config *intentionally* (so far) doesn't provide access to sections
because I thought we had to chose between using section names as path
(like in locations.conf) or arbitrary strings (like in bazaar.conf).

And I still think that we should promote the use of paths in all
configuration files, for compatibility we could still allows ALIASES and
BOOKMARKS but I think this kind of section can be embedded in the option
name instead. So your example above can *today* be written:

bzr.mergetool.default = meld
bzr.mergetool.meld = /usr/bin/meld %t %r %o
bzr.mergetool.kdiff3 = ...

and doing so allows *today* to have different definitions in
bazaar.conf, locations.conf and branch.conf.

Plugins would use their own dedicated name space:

qbzr.log.window_size = ...
qbzr.annotate.window_size = ...

Revision history for this message
Vincent Ladeuil (vila) wrote : Posted in a previous version of this proposal
Download full text (3.2 KiB)

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

    > On Thu, November 11, 2010 1:19 pm, Vincent Ladeuil wrote:
    >> > I'm not sure how I would actually go about splitting it. Branch from
    >> > lp:~doxxx/bzr/mergetools and then remove the extra bits from
    >> > lp:~doxxx/bzr/mergetools?
    >>
    >> Whatever works :) The above is ok. Then you'll have to decide whether
    >> you want to regularly merge from one branch to the other, use a pipeline
    >> or a loom. And don't forget to set the pre-requisite branch when
    >> submitting.

    > Won't the removal of extra bits in the original get merged into the second
    > when I try and merge new changes in the original into the second?

You have to create the second branch on top of the first one of course.

    > I already create option names like that but I didn't see a way to
    > query for options whose names start with a prefix, thus the
    > separate list option. I'll take another look at the config stuff
    > to see how to do this.

Look at the cmd_config implementation, _show_matching_options implements
such a filtering (with name='^bzr.mergetool.'), if you need it we should
certainly plan to make it available in a form reusable for your use
case, unless we provide a way to access such a set of options as a dict
(this will complicate the model (as in: should the dict be defined for
*each* config file OR be the union of the definitions in all config
files or something else)) but I think it's too soon to wander into such
complications now.

    > But yeah, if I can get this working then it does become more
    > user-editable, in which case I would be okay with removing the --add,
    > --update and --remove options from the mergetools command.

Cool.

    > Should these options go into a different section in the config
    > file? Or would that prevent putting them in locations.conf and
    > branch.conf?

Yes, using sections for that conflicts with using section names for
paths (see my other comment).

    > What about tool names which have "funny" characters in them like
    > spaces or other stuff? What's legal in an option name?

I haven't checked precisely what is legal and what is not, but python
identifiers are :) So no spaces, no funny names. I don't think it's a
problem.

<snip/>

    >> >> - _KOWN_MERGE_TOOLS really want to be a registry :)
    >>
    >> > I'll have a look at that.
    >>
    >> May be not :) If you rely on a naming rule for that...

    > It's just a place to list the names and command-lines of merge
    > tools that we already know about, so that we can try detect them
    > on the path. Is a registry suitable for that?

A registry will give you a place to store the default values that you
don't want to put in bazaar.conf and *this* is not implement today in
bzrlib.config.I'd like to have a config-like object that is *not*
associated to a file on disk but to a set of objects declarations so you
can say, for example:

kdiff3 = ConfigOption('bzr.mergetool.kdiff3, 'kdiff3 %b %t %o -o %r',
                      '''How kdiff3 should be called to ..blah.''')

and bzr config bzr.mergetool will display:
kdiff3 %b %t %o -o %r

i.e. this will define the...

Read more...

Revision history for this message
Gordon Tyler (doxxx) wrote : Posted in a previous version of this proposal

Okay, I think I've managed to extract all modifications to bzr commands. I've also made a few changes as suggested by Vincent.

Revision history for this message
Vincent Ladeuil (vila) wrote : Posted in a previous version of this proposal

@Gordon: Don't forget to put the state to NeedsReview when you feel ready for it.

Revision history for this message
Gordon Tyler (doxxx) wrote : Posted in a previous version of this proposal

On 11/13/2010 6:08 AM, Vincent Ladeuil wrote:
> @Gordon: Don't forget to put the state to NeedsReview when you feel ready for it.

I still need to double-check the stuff we discussed to make sure I
didn't miss anything. :)

Revision history for this message
Marius Kruger (amanica) wrote : Posted in a previous version of this proposal

On 11 November 2010 19:07, Gordon Tyler <email address hidden> wrote:
> On Thu, November 11, 2010 11:32 am, Vincent Ladeuil wrote:
>> - bzrlib.mergetools.py use DOS line endings, please use Unix line endings
>> (and watch for line with only spaces :-/),
>
> Sorry, I didn't know that it mattered. My editor is capable of reading DOS
> and Unix line endings, so I don't really notice whether its one or the
> other. I'll fix it and check if it has a trim spaces option (I think it
> does but I disabled it because somebody else complained a while back about
> spurious changes due to whitespace stripping).

You should not turn the trim spaces option on because then you will be
making spurious changes everywhere (unless it can be set to just trim
the lines you are editing). Currently we allow trailing whitespace but
we don't allow spurious changes allover the place. As far as I know
the rule is that reviewers should not complain about it because its
not important.

--
<>< Marius ><>

Revision history for this message
Gordon Tyler (doxxx) wrote : Posted in a previous version of this proposal

On 11/15/2010 10:26 AM, Marius Kruger wrote:
> On 11 November 2010 19:07, Gordon Tyler <email address hidden> wrote:
>> On Thu, November 11, 2010 11:32 am, Vincent Ladeuil wrote:
>>> - bzrlib.mergetools.py use DOS line endings, please use Unix line endings
>>> (and watch for line with only spaces :-/),
>>
>> Sorry, I didn't know that it mattered. My editor is capable of reading DOS
>> and Unix line endings, so I don't really notice whether its one or the
>> other. I'll fix it and check if it has a trim spaces option (I think it
>> does but I disabled it because somebody else complained a while back about
>> spurious changes due to whitespace stripping).
>
> You should not turn the trim spaces option on because then you will be
> making spurious changes everywhere (unless it can be set to just trim
> the lines you are editing). Currently we allow trailing whitespace but
> we don't allow spurious changes allover the place. As far as I know
> the rule is that reviewers should not complain about it because its
> not important.

Yeah, I discovered this and turned off the option and restored the
whitespace to what it was. I'll just have to be careful about leaving
spaces on empty lines.

Revision history for this message
Vincent Ladeuil (vila) wrote :
Download full text (4.5 KiB)

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

Read more...

review: Needs Fixing
Revision history for this message
Gordon Tyler (doxxx) wrote :
Download full text (4.7 KiB)

On Thu, November 25, 2010 4:46 am, Vincent Ladeuil wrote:
> - 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,

The MergeTool class *does* use a splitted commandline internally. The only
time it has to use _quote_args is to preserve spaces in arguments when
converting it into single string form for storage in the config.

> - 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 :-/)),

Agreed. I'll take care of this. I'll use full word tokens like {this}. I'm
also thinking of adding tokens like {this.name}, which is the basename of
the this file, for use with tools which can take additional parameters to
set window titles nicely.

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

The bzrlib.mergetools module itself is not user facing. I added stuff to
the what's new in the mergetools-command merge proposal since that
modifies user-facing commands.

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

Good point. I like my sugar but this can be removed.

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

Ok.

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

Ok.

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

I dislike the mon...

Read more...

Revision history for this message
Vincent Ladeuil (vila) wrote :
Download full text (4.8 KiB)

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

<snip/>

    > The MergeTool class *does* use a splitted commandline
    > internally. The only time it has to use _quote_args is to preserve
    > spaces in arguments when converting it into single string form for
    > storage in the config.

Argh, and we don't have reusable code for this use case ?

Err wait, why do we *need* to care here ? When do you need to go from a
list form to a line form ? All the user should have to deal with is the
line form where quoting should be respected (or bugs fixed if
bzrlib.cmdline is incorrect).

    >> - 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 :-/)),

    > Agreed. I'll take care of this. I'll use full word tokens like
    > {this}.

Cool.

    > I'm also thinking of adding tokens like {this.name}, which is the
    > basename of the this file, for use with tools which can take
    > additional parameters to set window titles nicely.

Can we postpone this ? :D

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

    > The bzrlib.mergetools module itself is not user facing. I added stuff to
    > the what's new in the mergetools-command merge proposal since that
    > modifies user-facing commands.

Config file content is user facing.

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

    > I dislike the monolithic-config-which-knows-all design of
    > bzrlib.config. It seems to me that it would be better to keep
    > bzrlib.config focused on providing generic access to the config
    > files with reusable functions for setting and getting various
    > types and structures of data in the config, while config code
    > specific to particular parts of bzr are actually located with the
    > code that uses it. Otherwise, code related to a particular
    > component, such as mergetools, is scattered across the codebase.

Yeah right, see my proposal, I don't like to define specific accessors
either, but that's what we have today. So to minimize the overall work,
it's better to either:
- fix the "wrong" API and then use it,
- use the "wrong" API and then fix the API and all its uses

This often means making a different proposal for the API change
too. While this may seem heavy weight this is also the guarantee that
people will use it instead of introducing their own.

But starting new APIs here and there is worse as it means we never get
to the new, better, APIs and instead spend time re-implementing the
...

Read more...

Revision history for this message
Gordon Tyler (doxxx) wrote :
Download full text (3.7 KiB)

On Thu, November 25, 2010 11:11 am, Vincent Ladeuil wrote:
>>>>>> Gordon Tyler <email address hidden> writes:
>
> <snip/>
>
> > The MergeTool class *does* use a splitted commandline
> > internally. The only time it has to use _quote_args is to preserve
> > spaces in arguments when converting it into single string form for
> > storage in the config.
>
> Argh, and we don't have reusable code for this use case ?
>
> Err wait, why do we *need* to care here ? When do you need to go from a
> list form to a line form ? All the user should have to deal with is the
> line form where quoting should be respected (or bugs fixed if
> bzrlib.cmdline is incorrect).

The user only has to deal with line form in the config. The quoting is
just for converting from split form in the MergeTool object into line form
for storing in the config.

Maybe what I should do is move this into the bzrlib.cmdline module to
provide a function which reverses the result of its split function?

> > I'm also thinking of adding tokens like {this.name}, which is the
> > basename of the this file, for use with tools which can take
> > additional parameters to set window titles nicely.
>
> Can we postpone this ? :D

Fine. :P

> >> - more NEWS entries :) This is a user facing change and as such
> >> also needs to be mentioned in the what's new,
>
> > The bzrlib.mergetools module itself is not user facing. I added
> stuff to
> > the what's new in the mergetools-command merge proposal since that
> > modifies user-facing commands.
>
> Config file content is user facing.

I suppose so. Except defining merge tools in the config does nothing
because there's nothing to use it. Yet. But I guess that's splitting
hairs. I'll move that stuff from the mergetools-commands proposal into
this one.

> Yeah right, see my proposal, I don't like to define specific accessors
> either, but that's what we have today. So to minimize the overall work,
> it's better to either:
> - fix the "wrong" API and then use it,
> - use the "wrong" API and then fix the API and all its uses
>
> This often means making a different proposal for the API change
> too. While this may seem heavy weight this is also the guarantee that
> people will use it instead of introducing their own.
>
> But starting new APIs here and there is worse as it means we never get
> to the new, better, APIs and instead spend time re-implementing the
> missing bits and re-fixing the same bugs :(

I see what you're trying to achieve. Okay, I'll move the config stuff into
bzrlib.config itself.

> >> - the %B related stuff is unclear (I wonder if it really need to
> >> addressed here instead of letting the caller handle it),
>
> > What %B stuff?
>
> Meh, %T. I mixed up TEMP and BASE as Gary mentioned the base file and %B
> in his comment.

It seems to be necessary for the meld tool. It's defined as 'meld %b %T %o'.

> >> Why not make the os.path.exists() call part of _find_executable() ?
>
> > One function to do one thing.
>
> Right, that doesn't forbid changing functions that are introduced to
> better fit their use cases :) Especially when they are used only once...

Point, I should jus...

Read more...

Revision history for this message
Andrew Bennetts (spiv) wrote :

 review needs-fixing

Gordon Tyler wrote:
[...]
> Point, I should just inline _find_executable. I kept it separate since I
> copied it from a public domain source, but I suppose I can modify it how I
> like.

“I copied it from a public domain source” worries me. You link to that source,
<http://snippets.dzone.com/posts/show/6313>, and I don't see anything there that
actually makes that snippet public domain. It explicitly credits a particular
person (as does your comment), and the page itself says “Copyright © 2007 DZone,
Inc.” Nothing on that page says that there is no copyright on that snippet. I
am not a lawyer, but I don't believe “code is shared publically without
explicitly spelled out conditions” can be assumed to imply “code is public
domain.”

So I don't think we can claim “Copyright (C) 2010 Canonical Ltd” for that code,
as your bzrlib/mergetools.py does. AFAICS you don't own the copyright, so you
can't assign it to Canonical Ltd, so we cannot make that claim.

As Vincent wrote, can we use or adapt the logic we already have in
ExecutableFeature._probe instead?

-Andrew.

review: Needs Fixing
Revision history for this message
Gordon Tyler (doxxx) wrote :

On 11/25/2010 5:28 PM, Andrew Bennetts wrote:
> So I don't think we can claim “Copyright (C) 2010 Canonical Ltd” for that code,
> as your bzrlib/mergetools.py does. AFAICS you don't own the copyright, so you
> can't assign it to Canonical Ltd, so we cannot make that claim.

Ok.

> As Vincent wrote, can we use or adapt the logic we already have in
> ExecutableFeature._probe instead?

It needs to use Windows PATH_EXT to test for '.exe', '.com', etc. but
otherwise it looks good. I'll extract a function from that into
bzrlib.osutils.

Revision history for this message
Gordon Tyler (doxxx) wrote :

I think I've addressed all of the comments so far. Please have another look and let me know if I've missed anything.

Revision history for this message
Gordon Tyler (doxxx) wrote :

I've made more changes according to vila's comments on https://code.launchpad.net/~doxxx/bzr/mergetools-commands/+merge/40924.

I'm not 100% sure I've got the treatment of known merge tools right. One problem is that with my modifications to qconfig, it now shows all the user-defined and pre-defined merge tools, and if the user clicks Okay, it's going to save all of them as user-defined, which negates the usefulness of having the pre-defined merge tools as vila described in the other mp. qconfig needs to be aware of the difference between a user-defined and a pre-defined merge tool. But at the same time, I don't want to make it hard for clients of the mergetools module and its functions on Config to use user-defined or pre-defined mergetools.

Revision history for this message
Vincent Ladeuil (vila) wrote :

> I've made more changes according to vila's comments on
> https://code.launchpad.net/~doxxx/bzr/mergetools-commands/+merge/40924.
>
> I'm not 100% sure I've got the treatment of known merge tools right. One
> problem is that with my modifications to qconfig, it now shows all the user-
> defined and pre-defined merge tools, and if the user clicks Okay, it's going
> to save all of them as user-defined,

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.

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

> which negates the usefulness of having
> the pre-defined merge tools as vila described in the other mp. qconfig needs
> to be aware of the difference between a user-defined and a pre-defined merge
> tool. But at the same time, I don't want to make it hard for clients of the
> mergetools module and its functions on Config to use user-defined or pre-
> defined mergetools.

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.

Revision history for this message
Gordon Tyler (doxxx) wrote :

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

Revision history for this message
Vincent Ladeuil (vila) wrote :
Download full text (3.4 KiB)

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

Read more...

Revision history for this message
Gordon Tyler (doxxx) wrote :

Okay, I've made changes according to the latest review and discussion with vila on IRC. Please have a look.

Revision history for this message
Vincent Ladeuil (vila) wrote :

Almost there (finally ;) !

36 + def get_default_merge_tool(self):
37 + return self.get_user_option('bzr.default_mergetool')
38 +

No need for that since there is no added check or feature there (and it shouldn't).

146 +
147 + def _get_command_line(self):
148 + return self._command_line
149 +
150 + def _set_command_line(self, command_line):
151 + self._command_line = command_line
152 + self._cmd_list = cmdline.split(self.command_line)
153 +
154 + command_line = property(_get_command_line, _set_command_line)

This isn't used in your proposal so it's not needed, if you need it for another mp, let's review it there please.

115 +from bzrlib import (
116 + cmdline,
117 + config,
118 + errors,
119 + osutils,
120 + trace,
121 + ui,
122 + workingtree,

Only cmdline and osutils seem to be necessary there.

Make sure you target 2.4 not 2.3 anymore (so move your news entries in the right files).

review: Needs Fixing
Revision history for this message
Gordon Tyler (doxxx) wrote :

I've simplified the mergetools module to just functions for checking availability and invoking a merge tool command line. There is no MergeTool class anymore. The config stuff just returns the command line string or a map of names to command lines.

Revision history for this message
Gordon Tyler (doxxx) wrote :

Fixed up release notes and what's new.

Revision history for this message
Vincent Ladeuil (vila) wrote :

Excellent, thanks for your persistent efforts there, this is now clearly defining the config options needed, outlining what is currently missing in our config framework.

Well done.

review: Approve
Revision history for this message
Vincent Ladeuil (vila) wrote :

sent to pqm by email

Revision history for this message
Vincent Ladeuil (vila) wrote :

sent to pqm by email

Revision history for this message
Vincent Ladeuil (vila) wrote :

@Gordon: pqm is rejecting this mp because of test failures, can you have a look at it ? (I don't get the email with the precise tests failing :-/)

Revision history for this message
John A Meinel (jameinel) wrote :

sent to pqm by email

Revision history for this message
John A Meinel (jameinel) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 01/21/2011 12:03 PM, Vincent Ladeuil wrote:
> @Gordon: pqm is rejecting this mp because of test failures, can you have a look at it ? (I don't get the email with the precise tests failing :-/)

I'll submit it, and I should get the failure email properly. Which can
help debugging.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk05+bkACgkQJdeBCYSNAANxHACbBD+uO3lWl+S6bReYFtw5rGS6
55YAn1HPkqYDaLgtYfQ1BkfXbKnaJcEn
=IPVA
-----END PGP SIGNATURE-----

Revision history for this message
Gordon Tyler (doxxx) wrote :

I'm also running the tests locally in parallel mode right now. Hopefully they'll finish in a reasonable amount of time.

I'll admit that I hadn't run the full selftest before submitting this. Mainly because it's never finished without errors on my Windows machine in parallel mode and it takes waaaay too long in normal mode. I only ran selftest on the tests that I changed.

/ashamed

Revision history for this message
John A Meinel (jameinel) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 01/21/2011 04:17 PM, Gordon Tyler wrote:
> I'm also running the tests locally in parallel mode right now. Hopefully they'll finish in a reasonable amount of time.
>
> I'll admit that I hadn't run the full selftest before submitting this. Mainly because it's never finished without errors on my Windows machine in parallel mode and it takes waaaay too long in normal mode. I only ran selftest on the tests that I changed.
>
> /ashamed
>

This is failing because you are using ".format" to do the formatting.
Which only exists in python 2.6/2.7. But not in 2.4/2.5.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk06BzMACgkQJdeBCYSNAAMzBgCghsQ3G/NeOWAkFhwE/RHsHP4a
zjAAn2eZKxxlycleO9ewWPN74wb3Nkpb
=2XhT
-----END PGP SIGNATURE-----

Revision history for this message
Gordon Tyler (doxxx) wrote :

I've made it python 2.4 compatible by using a *very simple* replacement formatting function. I didn't want to use 'blah %(base)' syntax because I liked the 'blah {base}' syntax better.

Revision history for this message
Gordon Tyler (doxxx) wrote :

Could somebody resend this to PQM if they think my py2.4 compatibility change is okay?

Revision history for this message
Vincent Ladeuil (vila) wrote :

sent to pqm by email

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/config.py'
2--- bzrlib/config.py 2011-01-12 18:12:58 +0000
3+++ bzrlib/config.py 2011-01-21 23:56:55 +0000
4@@ -82,6 +82,7 @@
5 errors,
6 lockdir,
7 mail_client,
8+ mergetools,
9 osutils,
10 registry,
11 symbol_versioning,
12@@ -357,6 +358,24 @@
13 else:
14 return True
15
16+ def get_merge_tools(self):
17+ tools = {}
18+ for (oname, value, section, conf_id, parser) in self._get_options():
19+ if oname.startswith('bzr.mergetool.'):
20+ tool_name = oname[len('bzr.mergetool.'):]
21+ tools[tool_name] = value
22+ trace.mutter('loaded merge tools: %r' % tools)
23+ return tools
24+
25+ def find_merge_tool(self, name):
26+ # We fake a defaults mechanism here by checking if the given name can
27+ # be found in the known_merge_tools if it's not found in the config.
28+ # This should be done through the proposed config defaults mechanism
29+ # when it becomes available in the future.
30+ command_line = (self.get_user_option('bzr.mergetool.%s' % name) or
31+ mergetools.known_merge_tools.get(name, None))
32+ return command_line
33+
34
35 class IniBasedConfig(Config):
36 """A configuration policy that draws from ini files."""
37
38=== modified file 'bzrlib/help_topics/en/configuration.txt'
39--- bzrlib/help_topics/en/configuration.txt 2010-12-07 09:06:39 +0000
40+++ bzrlib/help_topics/en/configuration.txt 2011-01-21 23:56:55 +0000
41@@ -584,3 +584,37 @@
42 If present, defines the ``--strict`` option default value for checking
43 uncommitted changes before sending a merge directive.
44
45+
46+External Merge Tools
47+--------------------
48+
49+bzr.mergetool.<name>
50+~~~~~~~~~~~~~~~~~~~~
51+
52+Defines an external merge tool called <name> with the given command-line.
53+Arguments containing spaces should be quoted using single or double quotes. The
54+executable may omit its path if it can be found on the PATH.
55+
56+The following markers can be used in the command-line to substitute filenames
57+involved in the merge conflict:
58+
59+{base} file.BASE
60+{this} file.THIS
61+{other} file.OTHER
62+{result} output file
63+{this_temp} temp copy of file.THIS, used to overwrite output file if merge
64+ succeeds.
65+
66+For example:
67+
68+ bzr.mergetool.kdiff3 = kdiff3 {base} {this} {other} -o {result}
69+
70+bzr.default_mergetool
71+~~~~~~~~~~~~~~~~~
72+
73+Specifies which external merge tool (as defined above) should be selected by
74+default in tools such as ``bzr qconflicts``.
75+
76+For example:
77+
78+ bzr.default_mergetool = kdiff3
79
80=== added file 'bzrlib/mergetools.py'
81--- bzrlib/mergetools.py 1970-01-01 00:00:00 +0000
82+++ bzrlib/mergetools.py 2011-01-21 23:56:55 +0000
83@@ -0,0 +1,116 @@
84+# Copyright (C) 2010 Canonical Ltd.
85+#
86+# This program is free software; you can redistribute it and/or modify
87+# it under the terms of the GNU General Public License as published by
88+# the Free Software Foundation; either version 2 of the License, or
89+# (at your option) any later version.
90+#
91+# This program is distributed in the hope that it will be useful,
92+# but WITHOUT ANY WARRANTY; without even the implied warranty of
93+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
94+# GNU General Public License for more details.
95+#
96+# You should have received a copy of the GNU General Public License
97+# along with this program; if not, write to the Free Software
98+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
99+
100+"""Utility functions for managing external merge tools such as kdiff3."""
101+
102+import os
103+import shutil
104+import subprocess
105+import sys
106+import tempfile
107+
108+from bzrlib.lazy_import import lazy_import
109+lazy_import(globals(), """
110+from bzrlib import (
111+ cmdline,
112+ osutils,
113+ trace,
114+)
115+""")
116+
117+
118+known_merge_tools = {
119+ 'bcompare': 'bcompare {this} {other} {base} {result}',
120+ 'kdiff3': 'kdiff3 {base} {this} {other} -o {result}',
121+ 'xdiff': 'xxdiff -m -O -M {result} {this} {base} {other}',
122+ 'meld': 'meld {base} {this_temp} {other}',
123+ 'opendiff': 'opendiff {this} {other} -ancestor {base} -merge {result}',
124+ 'winmergeu': 'winmergeu {result}',
125+}
126+
127+
128+def check_availability(command_line):
129+ cmd_list = cmdline.split(command_line)
130+ exe = cmd_list[0]
131+ if sys.platform == 'win32':
132+ if os.path.isabs(exe):
133+ base, ext = os.path.splitext(exe)
134+ path_ext = [unicode(s.lower())
135+ for s in os.getenv('PATHEXT', '').split(os.pathsep)]
136+ return os.path.exists(exe) and ext in path_ext
137+ else:
138+ return osutils.find_executable_on_path(exe) is not None
139+ else:
140+ return (os.access(exe, os.X_OK)
141+ or osutils.find_executable_on_path(exe) is not None)
142+
143+
144+def invoke(command_line, filename, invoker=None):
145+ """Invokes the given merge tool command line, substituting the given
146+ filename according to the embedded substitution markers. Optionally, it
147+ will use the given invoker function instead of the default
148+ subprocess_invoker.
149+ """
150+ if invoker is None:
151+ invoker = subprocess_invoker
152+ cmd_list = cmdline.split(command_line)
153+ args, tmp_file = _subst_filename(cmd_list, filename)
154+ def cleanup(retcode):
155+ if tmp_file is not None:
156+ if retcode == 0: # on success, replace file with temp file
157+ shutil.move(tmp_file, filename)
158+ else: # otherwise, delete temp file
159+ os.remove(tmp_file)
160+ return invoker(args[0], args[1:], cleanup)
161+
162+
163+def _subst_filename(args, filename):
164+ subst_names = {
165+ 'base': filename + u'.BASE',
166+ 'this': filename + u'.THIS',
167+ 'other': filename + u'.OTHER',
168+ 'result': filename,
169+ }
170+ tmp_file = None
171+ subst_args = []
172+ for arg in args:
173+ if '{this_temp}' in arg and not 'this_temp' in subst_names:
174+ fh, tmp_file = tempfile.mkstemp(u"_bzr_mergetools_%s.THIS" %
175+ os.path.basename(filename))
176+ trace.mutter('fh=%r, tmp_file=%r', fh, tmp_file)
177+ os.close(fh)
178+ shutil.copy(filename + u".THIS", tmp_file)
179+ subst_names['this_temp'] = tmp_file
180+ arg = _format_arg(arg, subst_names)
181+ subst_args.append(arg)
182+ return subst_args, tmp_file
183+
184+
185+# This would be better implemented using format() from python 2.6
186+def _format_arg(arg, subst_names):
187+ arg = arg.replace('{base}', subst_names['base'])
188+ arg = arg.replace('{this}', subst_names['this'])
189+ arg = arg.replace('{other}', subst_names['other'])
190+ arg = arg.replace('{result}', subst_names['result'])
191+ if subst_names.has_key('this_temp'):
192+ arg = arg.replace('{this_temp}', subst_names['this_temp'])
193+ return arg
194+
195+
196+def subprocess_invoker(executable, args, cleanup):
197+ retcode = subprocess.call([executable] + args)
198+ cleanup(retcode)
199+ return retcode
200
201=== modified file 'bzrlib/osutils.py'
202--- bzrlib/osutils.py 2011-01-12 21:06:32 +0000
203+++ bzrlib/osutils.py 2011-01-21 23:56:55 +0000
204@@ -2403,3 +2403,36 @@
205 except (ImportError, AttributeError):
206 # Either the fcntl module or specific constants are not present
207 pass
208+
209+
210+def find_executable_on_path(name):
211+ """Finds an executable on the PATH.
212+
213+ On Windows, this will try to append each extension in the PATHEXT
214+ environment variable to the name, if it cannot be found with the name
215+ as given.
216+
217+ :param name: The base name of the executable.
218+ :return: The path to the executable found or None.
219+ """
220+ path = os.environ.get('PATH')
221+ if path is None:
222+ return None
223+ path = path.split(os.pathsep)
224+ if sys.platform == 'win32':
225+ exts = os.environ.get('PATHEXT', '').split(os.pathsep)
226+ exts = [ext.lower() for ext in exts]
227+ base, ext = os.path.splitext(name)
228+ if ext != '':
229+ if ext.lower() not in exts:
230+ return None
231+ name = base
232+ exts = [ext]
233+ else:
234+ exts = ['']
235+ for ext in exts:
236+ for d in path:
237+ f = os.path.join(d, name) + ext
238+ if os.access(f, os.X_OK):
239+ return f
240+ return None
241
242=== modified file 'bzrlib/tests/__init__.py'
243--- bzrlib/tests/__init__.py 2011-01-12 18:39:25 +0000
244+++ bzrlib/tests/__init__.py 2011-01-21 23:56:55 +0000
245@@ -3802,6 +3802,7 @@
246 'bzrlib.tests.test_merge3',
247 'bzrlib.tests.test_merge_core',
248 'bzrlib.tests.test_merge_directive',
249+ 'bzrlib.tests.test_mergetools',
250 'bzrlib.tests.test_missing',
251 'bzrlib.tests.test_msgeditor',
252 'bzrlib.tests.test_multiparent',
253
254=== modified file 'bzrlib/tests/features.py'
255--- bzrlib/tests/features.py 2011-01-12 01:01:53 +0000
256+++ bzrlib/tests/features.py 2011-01-21 23:56:55 +0000
257@@ -19,7 +19,10 @@
258 import os
259 import stat
260
261-from bzrlib import tests
262+from bzrlib import (
263+ osutils,
264+ tests,
265+ )
266
267
268 class _NotRunningAsRoot(tests.Feature):
269@@ -113,16 +116,8 @@
270 return self._path
271
272 def _probe(self):
273- path = os.environ.get('PATH')
274- if path is None:
275- return False
276- for d in path.split(os.pathsep):
277- if d:
278- f = os.path.join(d, self.name)
279- if os.access(f, os.X_OK):
280- self._path = f
281- return True
282- return False
283+ self._path = osutils.find_executable_on_path(self.name)
284+ return self._path is not None
285
286 def feature_name(self):
287 return '%s executable' % self.name
288
289=== modified file 'bzrlib/tests/test_cmdline.py'
290--- bzrlib/tests/test_cmdline.py 2010-05-20 02:57:52 +0000
291+++ bzrlib/tests/test_cmdline.py 2011-01-21 23:56:55 +0000
292@@ -113,4 +113,3 @@
293 self.assertAsTokens([(False, r'\\"'), (False, r'*.py')],
294 r'\\\\\" *.py')
295 self.assertAsTokens([(True, u'\\\\')], u'"\\\\')
296-
297
298=== modified file 'bzrlib/tests/test_config.py'
299--- bzrlib/tests/test_config.py 2011-01-10 22:20:12 +0000
300+++ bzrlib/tests/test_config.py 2011-01-21 23:56:55 +0000
301@@ -33,6 +33,7 @@
302 errors,
303 osutils,
304 mail_client,
305+ mergetools,
306 ui,
307 urlutils,
308 tests,
309@@ -70,6 +71,9 @@
310 gpg_signing_command=gnome-gpg
311 log_format=short
312 user_global_option=something
313+bzr.mergetool.sometool=sometool {base} {this} {other} -o {result}
314+bzr.mergetool.funkytool=funkytool "arg with spaces" {this_temp}
315+bzr.default_mergetool=sometool
316 [ALIASES]
317 h=help
318 ll=""" + sample_long_alias + "\n"
319@@ -953,6 +957,41 @@
320 change_editor = my_config.get_change_editor('old', 'new')
321 self.assertIs(None, change_editor)
322
323+ def test_get_merge_tools(self):
324+ conf = self._get_sample_config()
325+ tools = conf.get_merge_tools()
326+ self.log(repr(tools))
327+ self.assertEqual(
328+ {u'funkytool' : u'funkytool "arg with spaces" {this_temp}',
329+ u'sometool' : u'sometool {base} {this} {other} -o {result}'},
330+ tools)
331+
332+ def test_get_merge_tools_empty(self):
333+ conf = self._get_empty_config()
334+ tools = conf.get_merge_tools()
335+ self.assertEqual({}, tools)
336+
337+ def test_find_merge_tool(self):
338+ conf = self._get_sample_config()
339+ cmdline = conf.find_merge_tool('sometool')
340+ self.assertEqual('sometool {base} {this} {other} -o {result}', cmdline)
341+
342+ def test_find_merge_tool_not_found(self):
343+ conf = self._get_sample_config()
344+ cmdline = conf.find_merge_tool('DOES NOT EXIST')
345+ self.assertIs(cmdline, None)
346+
347+ def test_find_merge_tool_known(self):
348+ conf = self._get_empty_config()
349+ cmdline = conf.find_merge_tool('kdiff3')
350+ self.assertEquals('kdiff3 {base} {this} {other} -o {result}', cmdline)
351+
352+ def test_find_merge_tool_override_known(self):
353+ conf = self._get_empty_config()
354+ conf.set_user_option('bzr.mergetool.kdiff3', 'kdiff3 blah')
355+ cmdline = conf.find_merge_tool('kdiff3')
356+ self.assertEqual('kdiff3 blah', cmdline)
357+
358
359 class TestGlobalConfigSavingOptions(tests.TestCaseInTempDir):
360
361
362=== added file 'bzrlib/tests/test_mergetools.py'
363--- bzrlib/tests/test_mergetools.py 1970-01-01 00:00:00 +0000
364+++ bzrlib/tests/test_mergetools.py 2011-01-21 23:56:55 +0000
365@@ -0,0 +1,167 @@
366+# Copyright (C) 2010 Canonical Ltd
367+#
368+# This program is free software; you can redistribute it and/or modify
369+# it under the terms of the GNU General Public License as published by
370+# the Free Software Foundation; either version 2 of the License, or
371+# (at your option) any later version.
372+#
373+# This program is distributed in the hope that it will be useful,
374+# but WITHOUT ANY WARRANTY; without even the implied warranty of
375+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
376+# GNU General Public License for more details.
377+#
378+# You should have received a copy of the GNU General Public License
379+# along with this program; if not, write to the Free Software
380+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
381+
382+import os
383+import re
384+import sys
385+import tempfile
386+
387+from bzrlib import (
388+ mergetools,
389+ tests
390+)
391+from bzrlib.tests.features import backslashdir_feature
392+
393+
394+class TestFilenameSubstitution(tests.TestCaseInTempDir):
395+
396+ def test_simple_filename(self):
397+ cmd_list = ['kdiff3', '{base}', '{this}', '{other}', '-o', '{result}']
398+ args, tmpfile = mergetools._subst_filename(cmd_list, 'test.txt')
399+ self.assertEqual(
400+ ['kdiff3',
401+ 'test.txt.BASE',
402+ 'test.txt.THIS',
403+ 'test.txt.OTHER',
404+ '-o',
405+ 'test.txt'],
406+ args)
407+
408+ def test_spaces(self):
409+ cmd_list = ['kdiff3', '{base}', '{this}', '{other}', '-o', '{result}']
410+ args, tmpfile = mergetools._subst_filename(cmd_list,
411+ 'file with space.txt')
412+ self.assertEqual(
413+ ['kdiff3',
414+ 'file with space.txt.BASE',
415+ 'file with space.txt.THIS',
416+ 'file with space.txt.OTHER',
417+ '-o',
418+ 'file with space.txt'],
419+ args)
420+
421+ def test_spaces_and_quotes(self):
422+ cmd_list = ['kdiff3', '{base}', '{this}', '{other}', '-o', '{result}']
423+ args, tmpfile = mergetools._subst_filename(cmd_list,
424+ 'file with "space and quotes".txt')
425+ self.assertEqual(
426+ ['kdiff3',
427+ 'file with "space and quotes".txt.BASE',
428+ 'file with "space and quotes".txt.THIS',
429+ 'file with "space and quotes".txt.OTHER',
430+ '-o',
431+ 'file with "space and quotes".txt'],
432+ args)
433+
434+ def test_tempfile(self):
435+ self.build_tree(('test.txt', 'test.txt.BASE', 'test.txt.THIS',
436+ 'test.txt.OTHER'))
437+ cmd_list = ['some_tool', '{this_temp}']
438+ args, tmpfile = mergetools._subst_filename(cmd_list, 'test.txt')
439+ self.failUnlessExists(tmpfile)
440+ os.remove(tmpfile)
441+
442+
443+class TestCheckAvailability(tests.TestCaseInTempDir):
444+
445+ def test_full_path(self):
446+ self.assertTrue(mergetools.check_availability(sys.executable))
447+
448+ def test_exe_on_path(self):
449+ self.assertTrue(mergetools.check_availability(
450+ os.path.basename(sys.executable)))
451+
452+ def test_nonexistent(self):
453+ self.assertFalse(mergetools.check_availability('DOES NOT EXIST'))
454+
455+ def test_non_executable(self):
456+ f, name = tempfile.mkstemp()
457+ try:
458+ self.log('temp filename: %s', name)
459+ self.assertFalse(mergetools.check_availability(name))
460+ finally:
461+ os.close(f)
462+ os.unlink(name)
463+
464+
465+class TestInvoke(tests.TestCaseInTempDir):
466+ def setUp(self):
467+ super(tests.TestCaseInTempDir, self).setUp()
468+ self._exe = None
469+ self._args = None
470+ self.build_tree_contents((
471+ ('test.txt', 'stuff'),
472+ ('test.txt.BASE', 'base stuff'),
473+ ('test.txt.THIS', 'this stuff'),
474+ ('test.txt.OTHER', 'other stuff'),
475+ ))
476+
477+ def test_success(self):
478+ def dummy_invoker(exe, args, cleanup):
479+ self._exe = exe
480+ self._args = args
481+ cleanup(0)
482+ return 0
483+ retcode = mergetools.invoke('tool {result}', 'test.txt', dummy_invoker)
484+ self.assertEqual(0, retcode)
485+ self.assertEqual('tool', self._exe)
486+ self.assertEqual(['test.txt'], self._args)
487+
488+ def test_failure(self):
489+ def dummy_invoker(exe, args, cleanup):
490+ self._exe = exe
491+ self._args = args
492+ cleanup(1)
493+ return 1
494+ retcode = mergetools.invoke('tool {result}', 'test.txt', dummy_invoker)
495+ self.assertEqual(1, retcode)
496+ self.assertEqual('tool', self._exe)
497+ self.assertEqual(['test.txt'], self._args)
498+
499+ def test_success_tempfile(self):
500+ def dummy_invoker(exe, args, cleanup):
501+ self._exe = exe
502+ self._args = args
503+ self.failUnlessExists(args[0])
504+ f = open(args[0], 'wt')
505+ f.write('temp stuff')
506+ f.close()
507+ cleanup(0)
508+ return 0
509+ retcode = mergetools.invoke('tool {this_temp}', 'test.txt',
510+ dummy_invoker)
511+ self.assertEqual(0, retcode)
512+ self.assertEqual('tool', self._exe)
513+ self.failIfExists(self._args[0])
514+ self.assertFileEqual('temp stuff', 'test.txt')
515+
516+ def test_failure_tempfile(self):
517+ def dummy_invoker(exe, args, cleanup):
518+ self._exe = exe
519+ self._args = args
520+ self.failUnlessExists(args[0])
521+ self.log(repr(args))
522+ f = open(args[0], 'wt')
523+ self.log(repr(f))
524+ f.write('temp stuff')
525+ f.close()
526+ cleanup(1)
527+ return 1
528+ retcode = mergetools.invoke('tool {this_temp}', 'test.txt',
529+ dummy_invoker)
530+ self.assertEqual(1, retcode)
531+ self.assertEqual('tool', self._exe)
532+ self.assertFileEqual('stuff', 'test.txt')
533
534=== modified file 'bzrlib/tests/test_osutils.py'
535--- bzrlib/tests/test_osutils.py 2011-01-12 15:50:12 +0000
536+++ bzrlib/tests/test_osutils.py 2011-01-21 23:56:55 +0000
537@@ -2119,3 +2119,25 @@
538 # revisited if we test against all implementations.
539 self.backups.remove('file.~2~')
540 self.assertBackupName('file.~2~', 'file')
541+
542+
543+class TestFindExecutableInPath(tests.TestCase):
544+
545+ def test_windows(self):
546+ if sys.platform != 'win32':
547+ raise tests.TestSkipped('test requires win32')
548+ self.assertTrue(osutils.find_executable_on_path('explorer') is not None)
549+ self.assertTrue(
550+ osutils.find_executable_on_path('explorer.exe') is not None)
551+ self.assertTrue(
552+ osutils.find_executable_on_path('EXPLORER.EXE') is not None)
553+ self.assertTrue(
554+ osutils.find_executable_on_path('THIS SHOULD NOT EXIST') is None)
555+ self.assertTrue(osutils.find_executable_on_path('file.txt') is None)
556+
557+ def test_other(self):
558+ if sys.platform == 'win32':
559+ raise tests.TestSkipped('test requires non-win32')
560+ self.assertTrue(osutils.find_executable_on_path('sh') is not None)
561+ self.assertTrue(
562+ osutils.find_executable_on_path('THIS SHOULD NOT EXIST') is None)
563
564=== modified file 'doc/en/release-notes/bzr-2.4.txt'
565--- doc/en/release-notes/bzr-2.4.txt 2011-01-21 23:21:18 +0000
566+++ doc/en/release-notes/bzr-2.4.txt 2011-01-21 23:56:55 +0000
567@@ -23,6 +23,9 @@
568 * The ``lp:`` directory service now supports Launchpad's QA staging.
569 (Jelmer Vernooij, #667483)
570
571+* External merge tools can now be configured in bazaar.conf. See
572+ ``bzr help configuration`` for more information. (Gordon Tyler, #489915)
573+
574 Improvements
575 ************
576
577@@ -73,6 +76,9 @@
578 .. Changes that may require updates in plugins or other code that uses
579 bzrlib.
580
581+* Added ``bzrlib.mergetools`` module with helper functions for working with
582+ the list of external merge tools. (Gordon Tyler, #489915)
583+
584 Internals
585 *********
586
587
588=== modified file 'doc/en/whats-new/whats-new-in-2.4.txt'
589--- doc/en/whats-new/whats-new-in-2.4.txt 2011-01-14 05:34:20 +0000
590+++ doc/en/whats-new/whats-new-in-2.4.txt 2011-01-21 23:56:55 +0000
591@@ -16,6 +16,12 @@
592 2.1, 2.2 and 2.3, and can read and write repositories generated by all
593 previous versions.
594
595+External Merge Tools
596+********************
597+
598+External merge tool configuration has been added to ``bzr`` core. The name
599+and commandline of one or more external merge tools can be defined in
600+bazaar.conf. See the help topic ``configuration`` for more details.
601
602 Further information
603 *******************