Merge lp:~baztian/bzr/recursive-diff into lp:bzr/2.5

[Bastian]

Support recursive diff: Show the differences using a custom diff program that supports directory based diff. I would love to have this in 2.5 and 2.6...

[Martin Packman (gz)]

This seems like a new feature, so it's too late for 2.5 really.

I'm not sure about using the template-like @recursive to trigger this, it seems instead like it should happen at the command level. Why did you go for that over a flag on the diff command?

[Vincent Ladeuil (vila)]

/me nods@mgz about 2.5 being closed for new features.

As 'meld' can already be used without the @recursive flag, I understand why
you put it in the --diff-options, but I too feel it weird. I think it would
be better to have a dedicated option that can be used *only* when --using is
also used, not sure about which name to use but since diff is already
recursive, may be --tree ?

Overall this is a nice feature to have and I'm glad you're working on it. I
wish we have a way to test it better than a review alone allows as I
suspect ignoring stuff you can't diff (in _write_recursive_files) is likely
to break without more testing. That's just a gut feeling though :-/

By the way, are you sure 'meld' can't display symlinks ?

_write_recursive_file, as a name, sounds funny and does not seem to match
the design.

It seems to me that you want to change DiffTree (or rather subclass it or
something) instead of DiffPath. I.e. really diff trees rather than hacking
how one file is diff'ed.

138 + def assert_diff(self, cmd_args, old_files, new_files):
139 + """Assert the correct files have been extracted for the
140 + revisions to the temp folders.

assertExtractedFiles would be more idiomatic.

150 + def assert_files(self, base_path, files):

Wow, this does a lot ! Worth a docstring and comments ;)

Should it be named assertProducedFiles or something ?

This seems to be a nice set of tests, but they are a bit hard too read. For
one, they seem to duplicate some setup that could go into a ... setUp()
method ;)

You may also want to add tests for symlinks (requiring features.Symlink) if only to check and document how your change behaves.

161 + if os.path.islink(filepath):

Someone knowing windows better than me can comment about whether this will
work or fail there ?

314 + def test_finish(self):

test_finish_cleans_up_temp_files ?

332 + def test_finish(self):

'Eager Test' (http://xunitpatterns.com/Obscure%20Test.html) is bad, you shouldn't do that :) 'Defect Localization' (http://xunitpatterns.com/Goals%20of%20Test%20Automation.html#Defect%20Localization) is good, do it !

Can you split it up with meaningful names ?

Please re-target to 2.6 and let's keep the discussion going, this really
sounds like a very cool feature ;)

review: Needs Resubmitting

[Martin Packman (gz)]

Rejecting for now, please resubmit against lp:bzr if you get the chance.

Unmerged revisions

6261. By Bastian on 2012-03-13

Show diffs using a custom diff program that supports directory based diff

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2012-02-16 16:42:43 +0000
+++ bzrlib/builtins.py	2012-03-13 17:48:37 +0000
@@ -2276,6 +2276,12 @@
         Show the differences using a custom diff program with options::
         
             bzr diff --using /usr/bin/diff --diff-options -wu
+
+        Show the differences using a custom diff program that supports
+        directory based diff::
+
+            bzr diff --using meld --diff-options @recursive
+
     """
     _see_also = ['status']
     takes_args = ['file*']

=== modified file 'bzrlib/diff.py'
--- bzrlib/diff.py	2011-12-18 12:46:49 +0000
+++ bzrlib/diff.py	2012-03-13 17:48:37 +0000
@@ -685,20 +685,26 @@
 
 class DiffFromTool(DiffPath):
 
+    _show_diff = False
+
     def __init__(self, command_template, old_tree, new_tree, to_file,
-                 path_encoding='utf-8'):
+                 path_encoding='utf-8', recursive=False):
         DiffPath.__init__(self, old_tree, new_tree, to_file, path_encoding)
         self.command_template = command_template
+        self._recursive = recursive
         self._root = osutils.mkdtemp(prefix='bzr-diff-')
 
     @classmethod
     def from_string(klass, command_string, old_tree, new_tree, to_file,
                     path_encoding='utf-8'):
         command_template = cmdline.split(command_string)
-        if '@' not in command_string:
+        recursive = '@recursive' in command_template
+        if recursive:
+            command_template.remove('@recursive')
+        if set(command_template).isdisjoint(('@old_path', '@new_path')):
             command_template.extend(['@old_path', '@new_path'])
         return klass(command_template, old_tree, new_tree, to_file,
-                     path_encoding)
+                     path_encoding, recursive)
 
     @classmethod
     def make_from_diff_tree(klass, command_string, external_diff_options=None):
@@ -712,7 +718,7 @@
 
     def _get_command(self, old_path, new_path):
         my_map = {'old_path': old_path, 'new_path': new_path}
-        command = [AtTemplate(t).substitute(my_map) for t in
+        command = [AtTemplate(t).safe_substitute(my_map) for t in
                    self.command_template]
         if sys.platform == 'win32': # Popen doesn't accept unicode on win32
             command_encoded = []
@@ -812,6 +818,21 @@
             osutils.make_readonly(full_path)
         return full_path
 
+    def _write_recursive_file(self, file_id, tree, prefix, relpath, kind):
+        if kind == 'file':
+            return self._write_file(file_id, tree, prefix, relpath,
+                                    force_temp=True)
+        else:
+            full_path = self._safe_filename(prefix, relpath)
+            if kind == 'directory':
+                dir_path = full_path
+            if not kind:
+                dir_path = osutils.dirname(full_path)
+            try:
+                os.makedirs(dir_path)
+            except OSError:
+                'directory already exists'
+
     def _prepare_files(self, file_id, old_path, new_path, force_temp=False,
                        allow_write_new=False):
         old_disk_path = self._write_file(file_id, self.old_tree, 'old',
@@ -822,6 +843,10 @@
         return old_disk_path, new_disk_path
 
     def finish(self):
+        if self._show_diff:
+            old_path = osutils.pathjoin(self._root, 'old')
+            new_path = osutils.pathjoin(self._root, 'new')
+            self._execute(old_path, new_path)
         try:
             osutils.rmtree(self._root)
         except OSError, e:
@@ -830,11 +855,21 @@
                         "cleanly removed: %s." % (self._root, e))
 
     def diff(self, file_id, old_path, new_path, old_kind, new_kind):
-        if (old_kind, new_kind) != ('file', 'file'):
-            return DiffPath.CANNOT_DIFF
-        (old_disk_path, new_disk_path) = self._prepare_files(
-                                                file_id, old_path, new_path)
-        self._execute(old_disk_path, new_disk_path)
+        if self._recursive:
+            allowed_kinds = ('directory', 'file', None)
+            if not (old_kind in allowed_kinds or new_kind in allowed_kinds):
+                return self.CANNOT_DIFF
+            self._show_diff = True
+            self._write_recursive_file(file_id, self.old_tree, 'old',
+                                       old_path, old_kind)
+            self._write_recursive_file(file_id, self.new_tree, 'new',
+                                       new_path, new_kind)
+        else:
+            if (old_kind, new_kind) != ('file', 'file'):
+                return DiffPath.CANNOT_DIFF
+            (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/blackbox/test_diff.py'
--- bzrlib/tests/blackbox/test_diff.py	2012-01-05 13:02:31 +0000
+++ bzrlib/tests/blackbox/test_diff.py	2012-03-13 17:48:37 +0000
@@ -26,6 +26,7 @@
     )
 from bzrlib.diff import (
     DiffTree,
+    DiffFromTool,
     format_registry as diff_format_registry,
     )
 from bzrlib.tests import (
@@ -414,6 +415,127 @@
         self.assertEquals('', err)
 
 
+class TestRecursiveDiff(DiffBase):
+
+    def assert_diff(self, cmd_args, old_files, new_files):
+        """Assert the correct files have been extracted for the
+        revisions to the temp folders.
+        """
+        def execute(other_self, old_path, new_path):
+            self.assert_files(old_path, old_files)
+            self.assert_files(new_path, new_files)
+        self.overrideAttr(DiffFromTool, '_execute', execute)
+        args = ['diff', '--using=foo', '--diff-options=@recursive']
+        args += cmd_args.split()
+        out, err = self.run_bzr(args, retcode=1)
+
+    def assert_files(self, base_path, files):
+        result = []
+        for (dirpath, dirnames, filenames) in os.walk(base_path):
+            dir = os.path.relpath(dirpath, base_path)
+            if not dirnames and not filenames:
+                # empty dir
+                if dirpath != base_path:
+                    result.append("%s/" % dir)
+                continue
+            for i in filenames + dirnames:
+                filepath = os.path.join(dirpath, i)
+                if os.path.islink(filepath):
+                    dest = os.path.join(dir, i)
+                    dest = os.path.relpath(dest)
+                    real_source = os.path.realpath(filepath)
+                    source = os.path.relpath(real_source, base_path)
+                    if os.path.isdir(filepath):
+                        source = "%s/" % source
+                    result.append((dest, source))
+                elif os.path.isfile(filepath):
+                    filename = os.path.join(dir, i)
+                    filename = os.path.relpath(filename)
+                    result.append(filename)
+        result = sorted(result)
+        expected = sorted(files)
+        self.assertEqual(result, expected)
+
+    def test_file_modified(self):
+        tree = self.make_branch_and_tree('test')
+        self.build_tree(('test/test.txt',))
+        tree.add('test.txt')
+        tree.commit('commit the tree')
+        self.build_tree_contents((('test/test.txt', 'changed file content'),))
+        tree.commit('file changed')
+        self.assert_diff('test -r 1..2', ('test.txt',), ('test.txt',))
+
+    def test_file_modified_and_removed(self):
+        tree = self.make_branch_and_tree('test')
+        self.build_tree(('test/test.txt',))
+        tree.add('test.txt')
+        tree.commit('commit the tree')
+        self.build_tree_contents((('test/test.txt', 'changed file content'),))
+        tree.commit('file changed')
+        tree.remove('test.txt')
+        tree.commit('file removed')
+        self.assert_diff('test -r 1..3', ('test.txt',), ())
+
+    def test_file_added(self):
+        tree = self.make_branch_and_tree('test')
+        self.build_tree(('test/test.txt',))
+        tree.add('test.txt')
+        tree.commit('commit the tree')
+        self.build_tree(('test/test2.txt',))
+        tree.add('test2.txt')
+        tree.commit('commit the tree 2')
+        self.assert_diff('test -r 1..2', (), ('test2.txt',))
+
+    def test_file_removed(self):
+        tree = self.make_branch_and_tree('test')
+        self.build_tree(('test/test.txt',))
+        tree.add('test.txt')
+        tree.commit('commit the tree')
+        tree.remove('test.txt')
+        tree.commit('commit the tree 2')
+        self.assert_diff('test -r 1..2', ('test.txt',), ())
+
+    def test_file_renamed(self):
+        tree = self.make_branch_and_tree('test')
+        self.build_tree(('test/test.txt',))
+        tree.add('test.txt')
+        tree.commit('commit the tree')
+        tree.rename_one('test.txt', 'test2.txt')
+        tree.commit('commit the tree 2')
+        self.assert_diff('test -r 1..2', ('test.txt',), ('test2.txt',))
+
+    def test_dir_renamed(self):
+        tree = self.make_branch_and_tree('test')
+        self.build_tree(('test/test1/', 'test/test1/test.txt',))
+        tree.add('test1')
+        tree.commit('commit the tree')
+        tree.rename_one('test1', 'test2')
+        tree.commit('commit the tree 2')
+        self.assert_diff('test -r 1..2', ('test1/',), ('test2/',))
+
+    def test_branches_with_working_tree(self):
+        tree1 = self.make_branch_and_tree('test1')
+        self.build_tree(('test1/test.txt',))
+        tree1.add('test.txt')
+        tree1.commit('commit the tree')
+        tree2 = tree1.bzrdir.sprout('test2').open_workingtree()
+        self.build_tree(('test2/test2.txt',))
+        tree2.add('test2.txt')
+        tree2.commit('commit the tree2')
+        self.assert_diff('test1 --new test2', (), ('test2.txt',))
+
+    def test_branches_branch2_without_working_tree(self):
+        tree1 = self.make_branch_and_tree('test1')
+        self.build_tree(('test1/test.txt',))
+        tree1.add('test.txt')
+        tree1.commit('commit the tree')
+        tree1.bzrdir.sprout('test2', create_tree_if_local=False)
+        self.build_tree(('test1/test2.txt',))
+        tree1.add('test2.txt')
+        tree1.commit('commit the tree2')
+        self.assert_diff('test1 --new test2', ('test2.txt',), ())
+
+
 class TestDiffOutput(DiffBase):
 
     def test_diff_output(self):

=== modified file 'bzrlib/tests/test_diff.py'
--- bzrlib/tests/test_diff.py	2012-01-25 21:13:15 +0000
+++ bzrlib/tests/test_diff.py	2012-03-13 17:48:37 +0000
@@ -1319,6 +1319,45 @@
         self.assertEqual(['C:\\Tools\\Diff.exe', 'old-path', 'new-path'],
                          diff_obj._get_command('old-path', 'new-path'))
 
+    def test_from_string_extra_matches(self):
+        diff_obj = diff.DiffFromTool.from_string('diff @ignoreme',
+                                                 None, None, None)
+        self.addCleanup(diff_obj.finish)
+        self.assertEqual(['diff', '@ignoreme', '@old_path', '@new_path'],
+                         diff_obj.command_template)
+        self.assertEqual(['diff', '@ignoreme', 'old-path', 'new-path'],
+                         diff_obj._get_command('old-path', 'new-path'))
+
+    def _pre_make_from_diff_tree(self, *args):
+        class DummyDiffTree(object):
+            old_tree = "old_tree"
+            new_tree = "new_tree"
+            to_file = "to_file"
+        diff_obj = diff.DiffFromTool.make_from_diff_tree(*args)(DummyDiffTree())
+        self.addCleanup(diff_obj.finish)
+        self.assertEqual(DummyDiffTree.old_tree, diff_obj.old_tree)
+        self.assertEqual(DummyDiffTree.new_tree, diff_obj.new_tree)
+        self.assertEqual(DummyDiffTree.to_file, diff_obj.to_file)
+        return diff_obj
+
+    def test_make_from_diff_tree(self):
+        diff_obj = self._pre_make_from_diff_tree('diff')
+        self.assertEqual(['diff', '@old_path', '@new_path'],
+                         diff_obj.command_template)
+        self.assertFalse(diff_obj._recursive)
+
+    def test_make_from_diff_tree_no_path_but_extra_option(self):
+        diff_obj = self._pre_make_from_diff_tree('diff', 'def')
+        self.assertEqual(['diff', 'def', '@old_path', '@new_path'],
+                         diff_obj.command_template)
+        self.assertFalse(diff_obj._recursive)
+
+    def test_make_from_diff_tree_recursive(self):
+        diff_obj = self._pre_make_from_diff_tree('diff', '@recursive')
+        self.assertEqual(['diff', '@old_path', '@new_path'],
+                         diff_obj.command_template)
+        self.assertTrue(diff_obj._recursive)
+
     def test_execute(self):
         output = StringIO()
         diff_obj = diff.DiffFromTool(['python', '-c',
@@ -1328,6 +1367,15 @@
         diff_obj._execute('old', 'new')
         self.assertEqual(output.getvalue().rstrip(), 'old new')
 
+    def test_finish(self):
+        output = StringIO()
+        diff_obj = diff.DiffFromTool(['python', '-c',
+                                      'print "@old_path @new_path"'],
+                                     None, None, output)
+        self.assertTrue(os.path.exists(diff_obj._root))
+        diff_obj.finish()
+        self.assertFalse(os.path.exists(diff_obj._root))
+
     def test_excute_missing(self):
         diff_obj = diff.DiffFromTool(['a-tool-which-is-unlikely-to-exist'],
                                      None, None, None)
@@ -1403,6 +1451,60 @@
         diff_obj._prepare_files('file2-id', 'oldname2', 'newname2')
 
 
+class TestDiffFromToolRecursive(tests.TestCaseWithTransport):
+
+    def test_finish(self):
+        self.old_tree = self.make_branch_and_tree('old-tree')
+        self.old_tree.lock_write()
+        self.addCleanup(self.old_tree.unlock)
+        self.new_tree = self.make_branch_and_tree('new-tree')
+        self.new_tree.lock_write()
+        self.addCleanup(self.new_tree.unlock)
+        self.build_tree_contents([('old-tree/olddir/',),
+                                  ('old-tree/olddir/oldfile', 'old\n'),
+                                  ('old-tree/oldfile2', 'old2\n')])
+        self.old_tree.add('olddir')
+        self.old_tree.add('olddir/oldfile', 'file-id')
+        self.old_tree.add('oldfile2', 'file-id2')
+        self.build_tree_contents([('new-tree/newdir/',),
+                                  ('new-tree/newdir/newfile', 'new\n'),
+                                  ('new-tree/newfile2', 'new2\n')])
+        self.new_tree.add('newdir')
+        self.new_tree.add('newdir/newfile', 'file-id')
+        self.new_tree.add('newfile2', 'file-id2')
+
+        output = StringIO()
+        diff_obj = diff.DiffFromTool(['python', '-c',
+                                      'print "Foo @old_path @new_path"'],
+                                     self.old_tree, self.new_tree,
+                                     output, recursive=True)
+        old_tmp_dir = os.path.join(diff_obj._root, 'old')
+        new_tmp_dir = os.path.join(diff_obj._root, 'new')
+
+        diff_obj.diff('file-id', 'olddir/oldfile', 'newdir/newfile',
+                      'file', 'file')
+        self.assertDirContents(old_tmp_dir, ['olddir'])
+        self.assertDirContents(new_tmp_dir, ['newdir'])
+        self.assertDirContents(os.path.join(old_tmp_dir, 'olddir'),
+                               ['oldfile'])
+        self.assertDirContents(os.path.join(new_tmp_dir, 'newdir'),
+                               ['newfile'])
+
+        diff_obj.diff('file-id2', 'oldfile2', 'newfile2', 'file', 'file')
+        self.assertDirContents(old_tmp_dir, ['olddir', 'oldfile2'])
+        self.assertDirContents(new_tmp_dir, ['newdir', 'newfile2'])
+        self.assertEqual(output.getvalue(), '')
+        diff_obj.finish()
+        self.assertEqual(output.getvalue().rstrip(),
+                         'Foo %s %s' % (old_tmp_dir, new_tmp_dir))
+        self.assertFalse(os.path.exists(old_tmp_dir))
+        self.assertFalse(os.path.exists(new_tmp_dir))
+
+    def assertDirContents(self, old_tmp_dir, contents):
+        old_list = sorted(os.listdir(old_tmp_dir))
+        self.assertEqual(contents, old_list)
+
+
 class TestDiffFromToolEncodedFilename(tests.TestCaseWithTransport):
 
     def test_encodable_filename(self):

=== modified file 'doc/en/release-notes/bzr-2.5.txt'
--- doc/en/release-notes/bzr-2.5.txt	2012-03-12 18:34:04 +0000
+++ doc/en/release-notes/bzr-2.5.txt	2012-03-13 17:48:37 +0000
@@ -18,7 +18,10 @@
 New Features
 ************
 
-.. New commands, options, etc that users may wish to try out.
+* If your external diff tool supports a directory based diff
+  (recursive diff) you can now specify the @recursive diff
+  option. This avoids opening a diff tool instance for every differing
+  file. (Bastian Bowe)
 
 Improvements
 ************