Merge lp:~jelmer/bzr/testament-tree into lp:bzr

[Jelmer Vernooij]

Make TestamentTree use a Tree rather than an Inventory.

Make Testament take a Tree rather than an Inventory.

[Martin Pool (mbp)]

I'm not generally a big fan of changing the expected parameter types for existing methods because it can lead to confusing failures in Python. There may not be an out-of-tree users of these though.

In order of decreasing complexity and decreasing niceness-to-callers:

* add a new method and retain but deprecate the old one.
* rename the methods/classes when you change their parameters
* check the argument type passed in and complain if it's an Inventory

the last is fine with me.

review: Needs Fixing

[Jelmer Vernooij (jelmer)]

Thanks for the review. :)

I've added a check to make sure the second argument is an instance of Tree.

[Martin Pool (mbp)]

Are there tests that will protect against this accidentally changing the generated values? Otherwise, all good.

review: Approve

[Jelmer Vernooij (jelmer)]

Yep, there are existing tests for testaments that assert specific testament strings are generated for specific trees.

[Jelmer Vernooij (jelmer)]

sent to pqm by email

=== modified file 'bzrlib/bundle/bundle_data.py'
--- bzrlib/bundle/bundle_data.py	2010-09-28 18:51:47 +0000
+++ bzrlib/bundle/bundle_data.py	2011-04-19 12:54:51 +0000
@@ -25,20 +25,22 @@
     osutils,
     timestamp,
     )
-import bzrlib.errors
 from bzrlib.bundle import apply_bundle
-from bzrlib.errors import (TestamentMismatch, BzrError,
-                           MalformedHeader, MalformedPatches, NotABundle)
-from bzrlib.inventory import (Inventory, InventoryEntry,
-                              InventoryDirectory, InventoryFile,
-                              InventoryLink)
-from bzrlib.osutils import sha_file, sha_string, pathjoin
+from bzrlib.errors import (
+    TestamentMismatch,
+    BzrError,
+    )
+from bzrlib.inventory import (
+    Inventory,
+    InventoryDirectory,
+    InventoryFile,
+    InventoryLink,
+    )
+from bzrlib.osutils import sha_string, pathjoin
 from bzrlib.revision import Revision, NULL_REVISION
 from bzrlib.testament import StrictTestament
 from bzrlib.trace import mutter, warning
-import bzrlib.transport
 from bzrlib.tree import Tree
-import bzrlib.urlutils
 from bzrlib.xml5 import serializer_v5
 
 
@@ -206,7 +208,7 @@
 
         inv = bundle_tree.inventory
         self._validate_inventory(inv, revision_id)
-        self._validate_revision(inv, revision_id)
+        self._validate_revision(bundle_tree, revision_id)
 
         return bundle_tree
 
@@ -286,7 +288,7 @@
             warning('Inventory sha hash mismatch for revision %s. %s'
                     ' != %s' % (revision_id, sha1, rev.inventory_sha1))
 
-    def _validate_revision(self, inventory, revision_id):
+    def _validate_revision(self, tree, revision_id):
         """Make sure all revision entries match their checksum."""
 
         # This is a mapping from each revision id to its sha hash
@@ -298,7 +300,7 @@
             raise AssertionError()
         if not (rev.revision_id == revision_id):
             raise AssertionError()
-        sha1 = self._testament_sha1(rev, inventory)
+        sha1 = self._testament_sha1(rev, tree)
         if sha1 != rev_info.sha1:
             raise TestamentMismatch(rev.revision_id, rev_info.sha1, sha1)
         if rev.revision_id in rev_to_sha1:
@@ -462,6 +464,7 @@
 
 
 class BundleTree(Tree):
+
     def __init__(self, base_tree, revision_id):
         self.base_tree = base_tree
         self._renamed = {} # Mapping from old_path => new_path
@@ -739,6 +742,23 @@
         for path, entry in self.inventory.iter_entries():
             yield entry.file_id
 
+    def list_files(self, include_root=False, from_dir=None, recursive=True):
+        # The only files returned by this are those from the version
+        inv = self.inventory
+        if from_dir is None:
+            from_dir_id = None
+        else:
+            from_dir_id = inv.path2id(from_dir)
+            if from_dir_id is None:
+                # Directory not versioned
+                return
+        entries = inv.iter_entries(from_dir=from_dir_id, recursive=recursive)
+        if inv.root is not None and not include_root and from_dir is None:
+            # skip the root for compatability with the current apis.
+            entries.next()
+        for path, entry in entries:
+            yield path, 'V', entry.kind, entry.file_id, entry
+
     def sorted_path_id(self):
         paths = []
         for result in self._new_id.iteritems():

=== modified file 'bzrlib/bundle/serializer/v08.py'
--- bzrlib/bundle/serializer/v08.py	2009-06-05 23:15:23 +0000
+++ bzrlib/bundle/serializer/v08.py	2011-04-19 12:54:51 +0000
@@ -553,5 +553,5 @@
         testament = StrictTestament.from_revision(repository, revision_id)
         return testament.as_sha1()
 
-    def _testament_sha1(self, revision, inventory):
-        return StrictTestament(revision, inventory).as_sha1()
+    def _testament_sha1(self, revision, tree):
+        return StrictTestament(revision, tree).as_sha1()

=== modified file 'bzrlib/bundle/serializer/v09.py'
--- bzrlib/bundle/serializer/v09.py	2009-03-23 14:59:43 +0000
+++ bzrlib/bundle/serializer/v09.py	2011-04-19 12:54:51 +0000
@@ -63,8 +63,8 @@
         testament = StrictTestament3.from_revision(repository, revision_id)
         return testament.as_sha1()
 
-    def _testament_sha1(self, revision, inventory):
-        return StrictTestament3(revision, inventory).as_sha1()
+    def _testament_sha1(self, revision, tree):
+        return StrictTestament3(revision, tree).as_sha1()
 
 
 class BundleReaderV09(BundleReader):

=== modified file 'bzrlib/repository.py'
--- bzrlib/repository.py	2011-04-17 18:24:56 +0000
+++ bzrlib/repository.py	2011-04-19 12:54:51 +0000
@@ -1163,7 +1163,9 @@
         if config is not None and config.signature_needed():
             if inv is None:
                 inv = self.get_inventory(revision_id)
-            plaintext = Testament(rev, inv).as_short_text()
+            tree = InventoryRevisionTree(self, inv, revision_id)
+            testament = Testament(rev, tree)
+            plaintext = testament.as_short_text()
             self.store_revision_signature(
                 gpg.GPGStrategy(config), plaintext, revision_id)
         # check inventory present

=== modified file 'bzrlib/testament.py'
--- bzrlib/testament.py	2009-03-23 14:59:43 +0000
+++ bzrlib/testament.py	2011-04-19 12:54:51 +0000
@@ -76,6 +76,7 @@
     contains_linebreaks,
     sha,
     )
+from bzrlib.tree import Tree
 
 
 class Testament(object):
@@ -91,23 +92,33 @@
 
     long_header = 'bazaar-ng testament version 1\n'
     short_header = 'bazaar-ng testament short form 1\n'
+    include_root = False
 
     @classmethod
     def from_revision(cls, repository, revision_id):
-        """Produce a new testament from a historical revision"""
+        """Produce a new testament from a historical revision."""
         rev = repository.get_revision(revision_id)
-        inventory = repository.get_inventory(revision_id)
-        return cls(rev, inventory)
-
-    def __init__(self, rev, inventory):
-        """Create a new testament for rev using inventory."""
+        tree = repository.revision_tree(revision_id)
+        return cls(rev, tree)
+
+    @classmethod
+    def from_revision_tree(cls, tree):
+        """Produce a new testament from a revision tree."""
+        rev = tree._repository.get_revision(tree.get_revision_id())
+        return cls(rev, tree)
+
+    def __init__(self, rev, tree):
+        """Create a new testament for rev using tree."""
         self.revision_id = rev.revision_id
         self.committer = rev.committer
         self.timezone = rev.timezone or 0
         self.timestamp = rev.timestamp
         self.message = rev.message
         self.parent_ids = rev.parent_ids[:]
-        self.inventory = inventory
+        if not isinstance(tree, Tree):
+            raise TypeError("As of bzr 2.4 Testament.__init__() takes a "
+                "Revision and a Tree.")
+        self.tree = tree
         self.revprops = copy(rev.properties)
         if contains_whitespace(self.revision_id):
             raise ValueError(self.revision_id)
@@ -143,9 +154,8 @@
         return [line.encode('utf-8') for line in r]
 
     def _get_entries(self):
-        entries = self.inventory.iter_entries()
-        entries.next()
-        return entries
+        return ((path, ie) for (path, versioned, kind, file_id, ie) in
+                self.tree.list_files(include_root=self.include_root))
 
     def _escape_path(self, path):
         if contains_linebreaks(path):
@@ -209,6 +219,7 @@
 
     long_header = 'bazaar-ng testament version 2.1\n'
     short_header = 'bazaar-ng testament short form 2.1\n'
+    include_root = False
     def _entry_to_line(self, path, ie):
         l = Testament._entry_to_line(self, path, ie)[:-1]
         l += ' ' + ie.revision
@@ -224,8 +235,7 @@
 
     long_header = 'bazaar testament version 3 strict\n'
     short_header = 'bazaar testament short form 3 strict\n'
-    def _get_entries(self):
-        return self.inventory.iter_entries()
+    include_root = True
 
     def _escape_path(self, path):
         if contains_linebreaks(path):

=== modified file 'bzrlib/tests/test_testament.py'
--- bzrlib/tests/test_testament.py	2010-11-05 20:54:32 +0000
+++ bzrlib/tests/test_testament.py	2011-04-19 12:54:51 +0000
@@ -22,7 +22,11 @@
 
 from bzrlib import osutils
 from bzrlib.tests import SymlinkFeature, TestCaseWithTransport
-from bzrlib.testament import Testament, StrictTestament, StrictTestament3
+from bzrlib.testament import (
+    Testament,
+    StrictTestament,
+    StrictTestament3,
+    )
 from bzrlib.transform import TreeTransform
 
 
@@ -136,10 +140,18 @@
         self.assertEqualDiff(
             self.expected('sample_unicode').encode('utf-8'), t.as_text())
 
+    def test_from_tree(self):
+        tree = self.b.repository.revision_tree('test@user-2')
+        testament = self.testament_class().from_revision_tree(tree)
+        text_1 = testament.as_short_text()
+        text_2 = self.from_revision(self.b.repository,
+                                    'test@user-2').as_short_text()
+        self.assertEqual(text_1, text_2)
+
     def test___init__(self):
         revision = self.b.repository.get_revision('test@user-2')
-        inventory = self.b.repository.get_inventory('test@user-2')
-        testament_1 = self.testament_class()(revision, inventory)
+        tree = self.b.repository.revision_tree('test@user-2')
+        testament_1 = self.testament_class()(revision, tree)
         text_1 = testament_1.as_short_text()
         text_2 = self.from_revision(self.b.repository,
                                     'test@user-2').as_short_text()

=== modified file 'doc/en/release-notes/bzr-2.4.txt'
--- doc/en/release-notes/bzr-2.4.txt	2011-04-19 04:37:48 +0000
+++ doc/en/release-notes/bzr-2.4.txt	2011-04-19 12:54:51 +0000
@@ -138,6 +138,9 @@
   on ``RepositoryFormat`` rather than a method on ``Repository``.
   (Jelmer Vernooij)
 
+* ``Testament`` now takes a ``tree`` rather than an
+  ``inventory``. (Jelmer Vernooij, #762608)
+
 * ``TestCase.failUnlessExists`` and ``failIfExists`` are deprecated in
   favour of ``assertPathExists`` and ``assertPathDoesNotExist`` 
   respectively.