Merge lp:~garyvdm/bzr/diff_using_gui into lp:bzr

[Gary van der Merwe]

This makes "diff --using" work better with gui diff apps.

This allows one to either run the diff command once, with a list of files to diff, e.g.:
bzr diff --using "meld [--diff]"

or to run the command once per file, but run the processes simultaneously, e.g.:
bzr diff --using "WinMergeU.exe /s &"

This fixes Bug 490212.

[lp:~garyvdm/bzr/diff_using_gui]

5172. By Gary van der Merwe on 2010-04-25

Update NEWS.

[Gordon Tyler (doxxx)]

Looks like there's a merge conflict in test_diff.

review: Needs Fixing

[lp:~garyvdm/bzr/diff_using_gui]

5173. By Gary van der Merwe on 2010-04-25

Merge bzr.dev.

[Gary van der Merwe (garyvdm)]

Thanks Gordon. Fixed. I did a uncommit, so you may need to do a pull --overwrite.

[Aaron Bentley (abentley)]

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

On 04/25/2010 11:40 AM, Gary van der Merwe wrote:
> This allows one to either run the diff command once, with a list of files to diff, e.g.:
> bzr diff --using "meld [--diff]"
>
> or to run the command once per file, but run the processes simultaneously, e.g.:
> bzr diff --using "WinMergeU.exe /s &"

What about the other way of using meld, where you specify the
directories to diff and the command recursively diffs them? The
original difftools plugin supported "fldiff", "kdiff3", "kompare",
"meld", "opendiff" and "xxdiff" as "TreeDiffTools".

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

iEYEARECAAYFAkvWUb4ACgkQ0F+nu1YWqI1AJgCfUhC2BYXThVDR7PF9bjRx+OE2
l6QAnRB1CBjF+1ebguEUJF2QcDxnBFgC
=bKRS
-----END PGP SIGNATURE-----

[Gary van der Merwe (garyvdm)]

On 27/04/2010 04:54, Aaron Bentley wrote:
> What about the other way of using meld, where you specify the
> directories to diff and the command recursively diffs them? The
> original difftools plugin supported "fldiff", "kdiff3", "kompare",
> "meld", "opendiff" and "xxdiff" as "TreeDiffTools".

I view that as a separate feature, which I've allready started working
on. I'm useing Jelmers new format option, which means to get that
behavior, you use:

  bzr diff --using meld --format ext-tree

[John A Meinel (jameinel)]

Having a mini-command line language to define what command to run seems a bit tricky, vs the original difftools method which just keeps a registry of known commands. I think Robert also agrees that communicating how to write a diff command is a bit hard to communicate. And telling people to just write a wrapper isn't seen as a reasonable solution on Windows.

As such, I'd rather see more of the difftools approach here, where you have a known list (registry) of tools that we know how to invoke on request.

248 + diff_obj = DiffFromTool(['python', '-c',
249 + 'print "@old_path @new_path"'],
250 + None, None, output,
251 + simultaneous_processes=True)

At a minimum, we should be using 'sys.executable' here, since you can't guaranteed that it is accessible simply as 'python'.

If you have simultaneous_process set, you should probably close proc.stdin. Otherwise you could be spawning multiple processes simultaneously that all want to read from stdin.

review: Needs Fixing

[lp:~garyvdm/bzr/diff_using_gui]

5174. By Gary van der Merwe on 2010-06-16

Merge bzr.dev.

5175. By Gary van der Merwe on 2010-06-16

Make tests pass (caused due to change in imports merged from bzr.dev.)

Unmerged revisions

5175. By Gary van der Merwe on 2010-06-16

Make tests pass (caused due to change in imports merged from bzr.dev.)

5174. By Gary van der Merwe on 2010-06-16

Merge bzr.dev.

5173. By Gary van der Merwe on 2010-04-25

Merge bzr.dev.

5172. By Gary van der Merwe on 2010-04-25

Update NEWS.

5171. By Gary van der Merwe on 2010-04-25

Test simultaneous_processes, and list_template behaviors.

5170. By Gary van der Merwe on 2010-04-25

Documentation for new --using usage.

5169. By Gary van der Merwe on 2010-04-25

Add tests for from_string.

5168. By Gary van der Merwe on 2010-04-25

Fix existing tests.

5167. By Gary van der Merwe on 2010-04-24

Add syntax for diff --using to launch with a list of files to diff.

5166. By Gary van der Merwe on 2010-04-24

Add syntax for diff --using to launch processes simultaneously.

=== modified file 'NEWS'
--- NEWS	2010-06-15 07:44:29 +0000
+++ NEWS	2010-06-16 09:03:28 +0000
@@ -22,6 +22,15 @@
 New Features
 ************
 
+* ``bzr diff --using`` now supports a options to work better with gui
+  diff apps. This allows one to either run the diff command once, with
+  a list of files to diff, e.g.:
+    bzr diff --using "meld [--diff]"
+  or to run the command once per file, but run the processes
+  simultaneously, e.g.:
+    bzr diff --using "WinMergeU.exe /s &"
+  (Gary van der Merwe, #490212)
+
 * Support ``--directory`` option for a number of additional commands:
   conflicts, merge-directive, missing, resolve, shelve, switch,
   unshelve, whoami. (Martin von Gagern, #527878)

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2010-06-07 02:38:04 +0000
+++ bzrlib/builtins.py	2010-06-16 09:03:28 +0000
@@ -1906,6 +1906,14 @@
         Same as 'bzr diff' but prefix paths with old/ and new/::
 
             bzr diff --prefix old/:new/
+        
+        Launch meld, with each file in a tab::
+        
+            bzr diff --using "meld [--diff]"
+        
+        Launch meld, with each file in a window::
+        
+            bzr diff --using "meld &"
     """
     _see_also = ['status']
     takes_args = ['file*']
@@ -1927,7 +1935,11 @@
         'revision',
         'change',
         Option('using',
-            help='Use this command to compare files.',
+            help='Use this command to compare files. This command will be run '
+                 'for each file that differs. To launch the command for each '
+                 'file simultaneously, suffex the command with "&". To launch '
+                 'command once, with a list of files to diff, wrap the '
+                 'arguments that need to be repeated in "[]". ',
             type=unicode,
             ),
         RegistryOption('format',

=== modified file 'bzrlib/diff.py'
--- bzrlib/diff.py	2010-05-25 17:27:52 +0000
+++ bzrlib/diff.py	2010-06-16 09:03:28 +0000
@@ -722,21 +722,42 @@
 
 
 class DiffFromTool(DiffPath):
-
     def __init__(self, command_template, old_tree, new_tree, to_file,
-                 path_encoding='utf-8'):
+                 path_encoding='utf-8', simultaneous_processes=False,
+                 list_template=None):
         DiffPath.__init__(self, old_tree, new_tree, to_file, path_encoding)
         self.command_template = command_template
         self._root = osutils.mkdtemp(prefix='bzr-diff-')
+        self._procs = []
+        self.simultaneous_processes = simultaneous_processes
+        self.list_template = list_template
+        if list_template:
+            self._command_list = []
 
     @classmethod
     def from_string(klass, command_string, old_tree, new_tree, to_file,
                     path_encoding='utf-8'):
-        command_template = cmdline.split(command_string)
+        list_template_match = re.match('(.*)\[(.*)\](.*)', command_string)
+        if not list_template_match:
+            command_template = cmdline.split(command_string)
+            list_template = None
+        else:
+            command_template = (cmdline.split(list_template_match.group(1)) +
+                                cmdline.split(list_template_match.group(3)))
+            list_template = cmdline.split(list_template_match.group(2))
+        
+        if command_template[-1] == '&':
+            simul = True
+            command_template = command_template[:-1]
+        else:
+            simul = False
+        
         if '@' not in command_string:
-            command_template.extend(['@old_path', '@new_path'])
+            (list_template or command_template).extend(['@old_path',
+                                                        '@new_path'])
         return klass(command_template, old_tree, new_tree, to_file,
-                     path_encoding)
+                     path_encoding, simultaneous_processes=simul,
+                     list_template=list_template)
 
     @classmethod
     def make_from_diff_tree(klass, command_string):
@@ -745,23 +766,32 @@
                                      diff_tree.new_tree, diff_tree.to_file)
         return from_diff_tree
 
-    def _get_command(self, old_path, new_path):
+    def _get_command(self, old_path, new_path, template):
         my_map = {'old_path': old_path, 'new_path': new_path}
-        return [AtTemplate(t).substitute(my_map) for t in
-                self.command_template]
+        return [AtTemplate(t).substitute(my_map) for t in template]
 
     def _execute(self, old_path, new_path):
-        command = self._get_command(old_path, new_path)
-        try:
-            proc = subprocess.Popen(command, stdout=subprocess.PIPE,
-                                    cwd=self._root)
-        except OSError, e:
-            if e.errno == errno.ENOENT:
-                raise errors.ExecutableMissing(command[0])
+        if not self.list_template:
+            command = self._get_command(old_path, new_path, self.command_template)
+            try:
+                proc = subprocess.Popen(command, stdout=subprocess.PIPE,
+                                        cwd=self._root)
+            except OSError, e:
+                if e.errno == errno.ENOENT:
+                    raise errors.ExecutableMissing(command[0])
+                else:
+                    raise
+            if self.simultaneous_processes:
+                self._procs.append(proc)
+                return
             else:
-                raise
-        self.to_file.write(proc.stdout.read())
-        return proc.wait()
+                self.to_file.write(proc.stdout.read())
+                return proc.wait()
+        else:
+            self._command_list.extend(
+                self._get_command(old_path, new_path,
+                                  self.list_template))
+            return
 
     def _try_symlink_root(self, tree, prefix):
         if (getattr(tree, 'abspath', None) is None
@@ -817,6 +847,24 @@
         return old_disk_path, new_disk_path
 
     def finish(self):
+        if self.list_template and self._command_list:
+            command = self.command_template + self._command_list
+            try:
+                proc = subprocess.Popen(command, stdout=subprocess.PIPE,
+                                        cwd=self._root)
+            except OSError, e:
+                if e.errno == errno.ENOENT:
+                    raise errors.ExecutableMissing(command[0])
+                else:
+                    raise
+            self.to_file.write(proc.stdout.read())
+            proc.wait()
+        
+        if self.simultaneous_processes:
+            for proc in self._procs:
+                self.to_file.write(proc.stdout.read())
+                proc.wait()
+        
         try:
             osutils.rmtree(self._root)
         except OSError, e:
@@ -831,6 +879,7 @@
                                                 file_id, old_path, new_path)
         self._execute(old_disk_path, new_disk_path)
 
+
     def edit_file(self, file_id):
         """Use this tool to edit a file.
 
@@ -846,7 +895,8 @@
                                            allow_write_new=True,
                                            force_temp=True)[1]
         command = self._get_command(osutils.pathjoin('old', old_path),
-                                    osutils.pathjoin('new', new_path))
+                                    osutils.pathjoin('new', new_path),
+                                    self.command_template)
         subprocess.call(command, cwd=self._root)
         new_file = open(new_abs_path, 'r')
         try:

=== modified file 'bzrlib/tests/test_diff.py'
--- bzrlib/tests/test_diff.py	2010-05-20 08:08:20 +0000
+++ bzrlib/tests/test_diff.py	2010-06-16 09:03:28 +0000
@@ -1287,6 +1287,7 @@
         self.addCleanup(diff_obj.finish)
         self.assertEqual(['diff', '@old_path', '@new_path'],
             diff_obj.command_template)
+        self.assertEqual(False, diff_obj.simultaneous_processes)
 
     def test_from_string_u5(self):
         diff_obj = diff.DiffFromTool.from_string('diff "-u 5"',
@@ -1295,7 +1296,25 @@
         self.assertEqual(['diff', '-u 5', '@old_path', '@new_path'],
                          diff_obj.command_template)
         self.assertEqual(['diff', '-u 5', 'old-path', 'new-path'],
-                         diff_obj._get_command('old-path', 'new-path'))
+                         diff_obj._get_command('old-path', 'new-path',
+                                               diff_obj.command_template))
+        
+    def test_from_string_simultaneous_processes(self):
+        diff_obj = diff.DiffFromTool.from_string('diff &', None, None, None)
+        self.addCleanup(diff_obj.finish)
+        self.assertEqual(['diff', '@old_path', '@new_path'],
+            diff_obj.command_template)
+        self.assertEqual(None, diff_obj.list_template)
+        self.assertEqual(True, diff_obj.simultaneous_processes)
+
+    def test_from_string_list(self):
+        diff_obj = diff.DiffFromTool.from_string('meld [--diff]', None, None, None)
+        self.addCleanup(diff_obj.finish)
+        self.assertEqual(['meld'],
+                         diff_obj.command_template)
+        self.assertEqual(['--diff', '@old_path', '@new_path'],
+                         diff_obj.list_template)
+        self.assertEqual(False, diff_obj.simultaneous_processes)
 
     def test_from_string_path_with_backslashes(self):
         self.requireFeature(features.backslashdir_feature)
@@ -1305,7 +1324,8 @@
         self.assertEqual(['C:\\Tools\\Diff.exe', '@old_path', '@new_path'],
                          diff_obj.command_template)
         self.assertEqual(['C:\\Tools\\Diff.exe', 'old-path', 'new-path'],
-                         diff_obj._get_command('old-path', 'new-path'))
+                         diff_obj._get_command('old-path', 'new-path',
+                                               diff_obj.command_template))
 
     def test_execute(self):
         output = StringIO()
@@ -1316,6 +1336,32 @@
         diff_obj._execute('old', 'new')
         self.assertEqual(output.getvalue().rstrip(), 'old new')
 
+    def test_execute_simultaneous_processes(self):
+        output = StringIO()
+        diff_obj = diff.DiffFromTool(['python', '-c',
+                                 'print "@old_path @new_path"'],
+                                None, None, output,
+                                simultaneous_processes=True)
+        diff_obj._execute('old1', 'new1')
+        diff_obj._execute('old2', 'new2')
+        self.assertEqual(output.getvalue().rstrip(), '')
+        diff_obj.finish()
+        self.assertEqual(output.getvalue().rstrip(), 'old1 new1\n'
+                                                     'old2 new2')
+
+    def test_execute_list(self):
+        output = StringIO()
+        diff_obj = diff.DiffFromTool(['python', '-c', 'import sys; '
+                                                 'print sys.argv[1:]'],
+                                None, None, output,
+                                list_template=['@old_path', '@new_path'])
+        diff_obj._execute('old1', 'new1')
+        diff_obj._execute('old2', 'new2')
+        self.assertEqual(output.getvalue().rstrip(), '')
+        diff_obj.finish()
+        self.assertEqual(output.getvalue().rstrip(), "['old1', 'new1', "
+                                                      "'old2', 'new2']")
+
     def test_excute_missing(self):
         diff_obj = diff.DiffFromTool(['a-tool-which-is-unlikely-to-exist'],
                                      None, None, None)