Rev 2845: Review feedback. in http://people.ubuntu.com/~robertc/baz2.0/repository

Robert Collins robertc at robertcollins.net
Fri Oct 19 01:57:21 BST 2007


At http://people.ubuntu.com/~robertc/baz2.0/repository

------------------------------------------------------------
revno: 2845
revision-id: robertc at robertcollins.net-20071019005716-owvnvr01kgswmeaq
parent: robertc at robertcollins.net-20071018013219-m98nsombwidgpd9d
committer: Robert Collins <robertc at robertcollins.net>
branch nick: repository
timestamp: Fri 2007-10-19 10:57:16 +1000
message:
  Review feedback.
modified:
  bzrlib/repofmt/pack_repo.py    pack_repo.py-20070813041115-gjv5ma7ktfqwsjgn-1
  bzrlib/tests/test_repository.py test_repository.py-20060131075918-65c555b881612f4d
=== modified file 'bzrlib/repofmt/pack_repo.py'
--- a/bzrlib/repofmt/pack_repo.py	2007-10-18 00:30:57 +0000
+++ b/bzrlib/repofmt/pack_repo.py	2007-10-19 00:57:16 +0000
@@ -324,6 +324,12 @@
         self.upload_transport.rename(self.random_name,
                 '../packs/' + self.name + '.pack')
         self._state = 'finished'
+        if 'fetch' in debug.debug_flags:
+            # XXX: size might be interesting?
+            mutter('%s: create_pack: pack renamed into place: %s%s->%s%s t+%6.3fs',
+                time.ctime(), self.upload_transport.base, self.random_name,
+                self.pack_transport, self.name,
+                time.time() - self.start_time)
 
     def make_index(self, index_type):
         """Construct a GraphIndex object for this packs index 'index_type'."""
@@ -700,17 +706,6 @@
             return None
         new_pack.finish()
         self.allocate(new_pack)
-        if 'fetch' in debug.debug_flags:
-            # XXX: size might be interesting?
-            mutter('%s: create_pack: pack renamed into place: %s%s->%s%s t+%6.3fs',
-                time.ctime(), self._upload_transport.base, new_pack.random_name,
-                new_pack.pack_transport, new_pack.name,
-                time.time() - new_pack.start_time)
-        if 'fetch' in debug.debug_flags:
-            # XXX: size might be interesting?
-            mutter('%s: create_pack: finished: %s%s t+%6.3fs',
-                time.ctime(), self._upload_transport.base, new_pack.random_name,
-                time.time() - new_pack.start_time)
         return new_pack
 
     def _execute_pack_operations(self, pack_operations):
@@ -1231,12 +1226,6 @@
 class GraphKnitRevisionStore(KnitRevisionStore):
     """An object to adapt access from RevisionStore's to use GraphKnits.
 
-    This should not live through to production: by production time we should
-    have fully integrated the new indexing and have new data for the
-    repository classes; also we may choose not to do a Knit1 compatible
-    new repository, just a Knit3 one. If neither of these happen, this 
-    should definately be cleaned up before merging.
-
     This class works by replacing the original RevisionStore.
     We need to do this because the GraphKnitRevisionStore is less
     isolated in its layering - it uses services from the repo.
@@ -1455,11 +1444,11 @@
         return self
 
     def _refresh_data(self):
-        if self._write_lock_count == 1 or self.control_files._lock_count==1:
+        if self._write_lock_count==1 or self.control_files._lock_count==1:
             # forget what names there are
             self._packs.reset()
-            # XXX: Better to do an in-memory merge, factor out code from
-            # _save_pack_names.
+            # XXX: Better to do an in-memory merge when acquiring a new lock -
+            # factor out code from _save_pack_names.
 
     def _start_write_group(self):
         self._packs._start_write_group()

=== modified file 'bzrlib/tests/test_repository.py'
--- a/bzrlib/tests/test_repository.py	2007-10-17 09:39:41 +0000
+++ b/bzrlib/tests/test_repository.py	2007-10-19 00:57:16 +0000
@@ -748,12 +748,6 @@
         # in case of side effects of locking.
         repo.lock_write()
         repo.unlock()
-        # we want:
-        # format 'Bazaar Experimental'
-        # lock: is a directory
-        # inventory.weave == empty_weave
-        # empty revision-store directory
-        # empty weaves directory
         t = repo.bzrdir.get_repository_transport(None)
         self.check_format(t)
         # XXX: no locks left when unlocked at the moment
@@ -793,21 +787,18 @@
             list(GraphIndex(t, 'pack-names', None).iter_all_entries()))
         self.assertTrue(S_ISDIR(t.stat('packs').st_mode))
         self.assertTrue(S_ISDIR(t.stat('upload').st_mode))
+        self.assertTrue(S_ISDIR(t.stat('indices').st_mode))
+        self.assertTrue(S_ISDIR(t.stat('obsolete_packs').st_mode))
 
     def test_shared_disk_layout(self):
         format = self.get_format()
         repo = self.make_repository('.', shared=True, format=format)
         # we want:
-        # format 'Bazaar-NG Knit Repository Format 1'
-        # lock: is a directory
-        # inventory.weave == empty_weave
-        # empty revision-store directory
-        # empty weaves directory
-        # a 'shared-storage' marker file.
         t = repo.bzrdir.get_repository_transport(None)
         self.check_format(t)
         # XXX: no locks left when unlocked at the moment
         # self.assertEqualDiff('', t.get('lock').read())
+        # We should have a 'shared-storage' marker file.
         self.assertEqualDiff('', t.get('shared-storage').read())
         self.check_databases(t)
 
@@ -816,18 +807,15 @@
         repo = self.make_repository('.', shared=True, format=format)
         repo.set_make_working_trees(False)
         # we want:
-        # format 'Bazaar-NG Knit Repository Format 1'
-        # lock ''
-        # inventory.weave == empty_weave
-        # empty revision-store directory
-        # empty weaves directory
-        # a 'shared-storage' marker file.
         t = repo.bzrdir.get_repository_transport(None)
         self.check_format(t)
         # XXX: no locks left when unlocked at the moment
         # self.assertEqualDiff('', t.get('lock').read())
+        # We should have a 'shared-storage' marker file.
         self.assertEqualDiff('', t.get('shared-storage').read())
+        # We should have a marker for the no-working-trees flag.
         self.assertEqualDiff('', t.get('no-working-trees').read())
+        # The marker should go when we toggle the setting.
         repo.set_make_working_trees(True)
         self.assertFalse(t.has('no-working-trees'))
         self.check_databases(t)
@@ -840,8 +828,9 @@
             list(GraphIndex(trans, 'pack-names', None).iter_all_entries()))
         tree.commit('foobarbaz')
         index = GraphIndex(trans, 'pack-names', None)
-        self.assertEqual(1, len(list(index.iter_all_entries())))
-        node = list(index.iter_all_entries())[0]
+        index_nodes = list(index.iter_all_entries())
+        self.assertEqual(1, len(index_nodes))
+        node = index_nodes[0]
         name = node[1][0]
         # the pack sizes should be listed in the index
         pack_value = node[2]
@@ -865,7 +854,7 @@
         trans = tree.branch.repository.bzrdir.get_repository_transport(None)
         # This test could be a little cheaper by replacing the packs
         # attribute on the repository to allow a different pack distribution
-        # and max packs policy - so we are hecking the policy is honoured
+        # and max packs policy - so we are checking the policy is honoured
         # in the test. But for now 11 commits is not a big deal in a single
         # test.
         for x in range(9):
@@ -898,7 +887,7 @@
         tree.commit('start')
         tree.commit('more work')
         tree.branch.repository.pack()
-        # there should be 1 packs:
+        # there should be 1 pack:
         index = GraphIndex(trans, 'pack-names', None)
         self.assertEqual(1, len(list(index.iter_all_entries())))
         self.assertEqual(2, len(tree.branch.repository.all_revision_ids()))
@@ -949,12 +938,13 @@
                 try:
                     r1.commit_write_group()
                 except:
+                    r1.abort_write_group()
                     r2.abort_write_group()
                     raise
                 r2.commit_write_group()
                 # tell r1 to reload from disk
                 r1._packs.reset()
-                # Now both repositories should now about both names
+                # Now both repositories should know about both names
                 r1._packs.ensure_loaded()
                 r2._packs.ensure_loaded()
                 self.assertEqual(r1._packs.names(), r2._packs.names())
@@ -1041,6 +1031,8 @@
         self.assertFalse(repo.get_physical_lock_status())
 
     def prepare_for_break_lock(self):
+        # Setup the global ui factory state so that a break-lock method call
+        # will find usable input in the input stream.
         old_factory = bzrlib.ui.ui_factory
         def restoreFactory():
             bzrlib.ui.ui_factory = old_factory
@@ -1083,7 +1075,7 @@
         return bzrdir.format_registry.make_bzrdir('experimental')
 
     def test__max_pack_count(self):
-        """The maximum pack count is geared from the number of revisions."""
+        """The maximum pack count is a function of the number of revisions."""
         format = self.get_format()
         repo = self.make_repository('.', format=format)
         packs = repo._packs
@@ -1242,8 +1234,7 @@
         sig_index = GraphIndex(packs._index_transport, name + '.six',
             packs._names[name][3])
         self.assertEqual(pack_repo.ExistingPack(packs._pack_transport,
-                packs.names()[0], rev_index, inv_index, txt_index, sig_index),
-            pack_1)
+            name, rev_index, inv_index, txt_index, sig_index), pack_1)
         # and the same instance should be returned on successive calls.
         self.assertTrue(pack_1 is packs.get_pack_by_name(name))
 



More information about the bazaar-commits mailing list