[MERGE] Add support for previewing the result of a merge

[John Arbash Meinel]

Aaron Bentley wrote:
> Robert Collins wrote:
>> On Wed, 2007-12-26 at 21:36 -0500, Aaron Bentley wrote:
>>> This patch adds support for previewing the result of a merge or other
>>> TreeTransform.
> 
>> Does this add a UI to use it? (e.g. bzr merge --preview). It would be
>> great to be able to generate a history sensitive diff between arbitrary
>> branches (better than -r ancestor: does) by using this from the command
>> line.
> 
> Okay, here's a version that includes that functionality in merge.  (Not
> diff, because it's more important to vary the merge parameters than the
> diff parameters).
> 
> Aaron
> 

I really like having this functionality, though I'm wondering about the
proper way to expose it.

I'm a little hesitant to change a mutating command into a preview
command with a flag, but only because we don't have a lot of precedence
for it. I think we've talked about having a "pull -n" (or whatever)
rather than "missing". And I like not having an extra "bzr
preview-merge" command. Though "bzr preview-merge" might be more
discoverable than "bzr merge --preview".

Though it also makes sense to not create another command that has to
duplicate all of the options of merge.

It is also technically a backwards incompatible change to the Merger
api, since now do_merge must be handled. (Then again, I never really
liked changing the tree contents as part of __init__()).

...

> +    def test_iter_changes(self):
> +        revision_tree = self.create_tree()
> +        root = revision_tree.inventory.root.file_id
> +        preview = TransformPreview(revision_tree)
> +        a_trans_id = preview.trans_id_file_id('a-id')
> +        preview.delete_contents(a_trans_id)
> +        preview.create_file('b content', a_trans_id)
> +        preview_tree = preview.get_preview_tree()
> +        self.assertEqual([('a-id', ('a', 'a'), True, (True, True),
> +                          (root, root), ('a', 'a'), ('file', 'file'),
> +                          (False, False))],
> +                          list(preview_tree._iter_changes(revision_tree)))
> +        # Test unsupported parameters
> +        e = self.assertRaises(ValueError, preview_tree._iter_changes,
> +                              preview_tree)
> +        self.assertEqual('from_tree must be transform source tree.', str(e))
> +        e = self.assertRaises(ValueError, preview_tree._iter_changes,
> +                              revision_tree, include_unchanged=True)
> +        self.assertEqual('include_unchanged is not supported', str(e))
> +        e = self.assertRaises(ValueError, preview_tree._iter_changes,
> +                              revision_tree, specific_files=['pete'])
> +        self.assertEqual('specific_files is not supported', str(e))
> +        e = self.assertRaises(ValueError, preview_tree._iter_changes,
> +                              revision_tree, want_unversioned=True)
> +        self.assertEqual('want_unversioned is not supported', str(e))
> +        # test ignored parameters
> +
> +        # extra_trees is harmless without specific_files, so we'll silently
> +        # accept it, even though we won't use it.
> +        preview_tree._iter_changes(revision_tree, extra_trees=[preview_tree])
> +        # require_versioned is meaningless without specific_files.
> +        preview_tree._iter_changes(revision_tree, require_versioned=False)
> +        # pb could be supported, but TT.iter_changes doesn't support it.
> +        preview_tree._iter_changes(revision_tree, pb=progress.DummyProgress())
> +

^- This seems like a test that you would really rather have as a set of
tests, rather than one big conglomeration.
Also, shouldn't you be iterating over the results? Otherwise it is only
checking to the point of the first 'yield'.

We do have the helper "assertListRaises" which uses list(callable(*args,
**kwargs)) rather than just "callable(*args, **kwargs)". Though that
should be updated to return the captured error.

=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py    2007-12-06 12:03:35 +0000
+++ bzrlib/tests/__init__.py    2008-01-02 17:00:33 +0000
@@ -930,8 +930,8 @@
         """
         try:
             list(func(*args, **kwargs))
-        except excClass:
-            return
+        except excClass, e:
+            return e
         else:
             if getattr(excClass,'__name__', None) is not None:
                 excName = excClass.__name__

...
+class TreeTransformBase(object):
+
+    def __init__(self, tree, limbodir, pb=DummyProgress(),

^- You should at least have a minimal doc string here.

...

+class TransformPreview(TreeTransformBase):
+


And a nice descriptive one here.

...
+        for child in self._tree.inventory[file_id].children.iterkeys():
+            childpath = joinpath(path, child)
+            yield self.trans_id_tree_path(childpath)
+
+class _PreviewTree(object):
+

^- 2 blank lines

_PreviewTree doesn't seem to support the full "Tree" interface
(get_file_lines, get_file_text, for example). It probably implements
enough for show_diff_trees(), but I'm wondering if we should be trying
to add it to the tree_implementations tests. Right *now* we don't need
it. But as we find this being useful, I think it is going to get more
people wanting it to act just like any other tree. (Especially if we
create a revision spec like "bzr diff -r merge-preview:../other/branch".)

So for the moment, I'm:
BB:comment

I didn't review all of the line-by-line code yet, and mostly wanted to
discuss overall concepts.

John
=:->

PS> It is interesting how you got *really* productive over the holidays,
while the rest of us sort of disappeared. My laptop's power supply died,
which didn't help. Thanks for all the effort, though.