Merge lp:~garyvdm/bzr/extdiff-no-tmp-for-wt into lp:bzr

[Gary van der Merwe]

[Gary van der Merwe (garyvdm)]

This patch changes bzr behaviour when launching a external diff tool via bzr diff --using. If the file is from a working tree, it does not create a temporary file, rather, it just passes the path to that file in the working tree. This saves writing the file to the disk, but more importantly, allows one to edit the file in diff tools that allow editing, such as meld.

[Martin Pool (mbp)]

That's a very nice improvement, and very simple. I recall someone saying there were complications, but if this works for at least some cases, that's great.

review: Approve

[John A Meinel (jameinel)]

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

Martin Pool wrote:
> Review: Approve
> That's a very nice improvement, and very simple. I recall someone saying there were complications, but if this works for at least some cases, that's great.

It seems to ignore the "force_temp" flag. Would it be better to have:

if not force_temp and isinstance(tree, WorkingTree):
?

John
=:->

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

iEYEARECAAYFAksVlkcACgkQJdeBCYSNAAORBQCfUKV3Lq1vMuwIBBPGqvp5CD4o
QesAoMVWF+iWA/qqufVMN/xKivyZlTLr
=L/NR
-----END PGP SIGNATURE-----

[Gary van der Merwe (garyvdm)]

Gary van der Merwe wrote on 2009-12-01

> This patch changes bzr behaviour when launching a external diff tool via bzr
> diff --using. If the file is from a working tree, it does not create a
> temporary file, rather, it just passes the path to that file in the working
> tree. This saves writing the file to the disk, but more importantly, allows
> one to edit the file in diff tools that allow editing, such as meld.

This will now still write the temp file if force_temp is true. There are no tests for this, but it currently is not used from anywhere.

[John A Meinel (jameinel)]

This seems better than what we currently have. So we should merge it as is.

What about deleted files, though?

Also, self._execute looks like it is using relative paths. Do you know if this still works if you are in a subdir of the working tree, etc?

review: Approve

[Martin Pool (mbp)]

review: Approve

[Martin Pool (mbp)]

sent to pqm

=== modified file 'NEWS'
--- NEWS	2009-12-03 02:24:54 +0000
+++ NEWS	2009-12-03 07:40:28 +0000
@@ -57,6 +57,12 @@
   new rich-root heads. This can cut a conversion time in half (mysql from
   13.5h => 6.2h) (John Arbash Meinel, #487632)
 
+* When launching a external diff tool via bzr diff --using, temporary files
+  are no longer created, rather, the path to the file in the working tree is
+  passed to the external diff tool. This allows the file to be edited if the
+  diff tool provides for this. (Gary van der Merwe, #490738)
+  
+
 Improvements
 ************
 
 
=== modified file 'bzrlib/diff.py'
--- bzrlib/diff.py	2009-11-03 01:15:09 +0000
+++ bzrlib/diff.py	2009-12-03 07:40:28 +0000
@@ -39,6 +39,8 @@
     timestamp,
     views,
     )
+
+from bzrlib.workingtree import WorkingTree
 """)
 
 from bzrlib.symbol_versioning import (
@@ -722,6 +724,9 @@
 
     def _write_file(self, file_id, tree, prefix, relpath, force_temp=False,
                     allow_write=False):
+        if not force_temp and isinstance(tree, WorkingTree):
+            return tree.abspath(tree.id2path(file_id))
+        
         full_path = osutils.pathjoin(self._root, prefix, relpath)
         if not force_temp and self._try_symlink_root(tree, prefix):
             return full_path
@@ -766,9 +771,9 @@
     def diff(self, file_id, old_path, new_path, old_kind, new_kind):
         if (old_kind, new_kind) != ('file', 'file'):
             return DiffPath.CANNOT_DIFF
-        self._prepare_files(file_id, old_path, new_path)
-        self._execute(osutils.pathjoin('old', old_path),
-                      osutils.pathjoin('new', new_path))
+        (old_disk_path, new_disk_path) = self._prepare_files(
+                                                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.
 
=== modified file 'bzrlib/tests/test_diff.py'
--- bzrlib/tests/test_diff.py	2009-10-31 01:16:23 +0000
+++ bzrlib/tests/test_diff.py	2009-12-03 07:40:28 +0000
@@ -1378,7 +1378,7 @@
                                                      'newname')
         self.assertContainsRe(old_path, 'old/oldname$')
         self.assertEqual(0, os.stat(old_path).st_mtime)
-        self.assertContainsRe(new_path, 'new/newname$')
+        self.assertContainsRe(new_path, 'tree/newname$')
         self.assertFileEqual('oldcontent', old_path)
         self.assertFileEqual('newcontent', new_path)
         if osutils.host_os_dereferences_symlinks():