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

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

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.

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

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

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

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

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

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?

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

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

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

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

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

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

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.

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?

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

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

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

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?

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

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.

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

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

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

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.

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

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.

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

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

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

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?

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

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

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

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

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

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

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.