Merge lp:~jelmer/bzr/move-write-group into lp:bzr

[Jelmer Vernooij]

Move write group infrastructure to bzrlib.vf_repository..

[Jelmer Vernooij (jelmer)]

The only semi-generic thing that still requires the use of write groups is Repository.sign_revision(). I've filed bug 781751 about this.

[Jelmer Vernooij (jelmer)]

Actually, I'm going to wait with this. There is too much code that still relies on start_write_group/abort_write_group/commit_write_group at the moment, and moving them right now doesn't gain us anything.

=== modified file 'bzrlib/repofmt/knitrepo.py'
--- bzrlib/repofmt/knitrepo.py	2011-05-08 11:34:17 +0000
+++ bzrlib/repofmt/knitrepo.py	2011-05-12 15:41:07 +0000
@@ -35,11 +35,11 @@
 from bzrlib.decorators import needs_read_lock, needs_write_lock
 from bzrlib.repository import (
     InterRepository,
-    IsInWriteGroupError,
     RepositoryFormat,
     )
 from bzrlib.vf_repository import (
     InterSameDataRepository,
+    IsInWriteGroupError,
     MetaDirVersionedFileRepository,
     MetaDirVersionedFileRepositoryFormat,
     VersionedFileCommitBuilder,

=== modified file 'bzrlib/repository.py'
--- bzrlib/repository.py	2011-05-09 12:01:12 +0000
+++ bzrlib/repository.py	2011-05-12 15:41:07 +0000
@@ -46,21 +46,15 @@
 from bzrlib.inter import InterObject
 from bzrlib.lock import _RelockDebugMixin, LogicalLockResult
 from bzrlib.trace import (
-    log_exception_quietly, note, mutter, mutter_callsite, warning)
+    mutter_callsite,
+    warning,
+    )
 
 
 # Old formats display a warning, but only once
 _deprecation_warning_done = False
 
 
-class IsInWriteGroupError(errors.InternalBzrError):
-
-    _fmt = "May not refresh_data of repo %(repo)s while in a write group."
-
-    def __init__(self, repo):
-        errors.InternalBzrError.__init__(self, repo=repo)
-
-
 class CommitBuilder(object):
     """Provides an interface to build up a commit.
 
@@ -252,49 +246,6 @@
     base class for most Bazaar repositories.
     """
 
-    def abort_write_group(self, suppress_errors=False):
-        """Commit the contents accrued within the current write group.
-
-        :param suppress_errors: if true, abort_write_group will catch and log
-            unexpected errors that happen during the abort, rather than
-            allowing them to propagate.  Defaults to False.
-
-        :seealso: start_write_group.
-        """
-        if self._write_group is not self.get_transaction():
-            # has an unlock or relock occured ?
-            if suppress_errors:
-                mutter(
-                '(suppressed) mismatched lock context and write group. %r, %r',
-                self._write_group, self.get_transaction())
-                return
-            raise errors.BzrError(
-                'mismatched lock context and write group. %r, %r' %
-                (self._write_group, self.get_transaction()))
-        try:
-            self._abort_write_group()
-        except Exception, exc:
-            self._write_group = None
-            if not suppress_errors:
-                raise
-            mutter('abort_write_group failed')
-            log_exception_quietly()
-            note('bzr: ERROR (ignored): %s', exc)
-        self._write_group = None
-
-    def _abort_write_group(self):
-        """Template method for per-repository write group cleanup.
-
-        This is called during abort before the write group is considered to be
-        finished and should cleanup any internal state accrued during the write
-        group. There is no requirement that data handed to the repository be
-        *not* made available - this is not a rollback - but neither should any
-        attempt be made to ensure that data added is fully commited. Abort is
-        invoked when an error has occured so futher disk or network operations
-        may not be possible or may error and if possible should not be
-        attempted.
-        """
-
     def add_fallback_repository(self, repository):
         """Add a repository to use for looking up data not held locally.
 
@@ -373,8 +324,6 @@
         self.control_files = control_files
         self._transport = control_files._transport
         self.base = self._transport.base
-        # for tests
-        self._write_group = None
         # Additional places to query for data.
         self._fallback_repositories = []
 
@@ -418,13 +367,6 @@
             return False
         return (self._transport.base == other._transport.base)
 
-    def is_in_write_group(self):
-        """Return True if there is an open write group.
-
-        :seealso: start_write_group.
-        """
-        return self._write_group is not None
-
     def is_locked(self):
         return self.control_files.is_locked()
 
@@ -624,59 +566,6 @@
         """
         return InterRepository.get(self, destination).copy_content(revision_id)
 
-    def commit_write_group(self):
-        """Commit the contents accrued within the current write group.
-
-        :seealso: start_write_group.
-        
-        :return: it may return an opaque hint that can be passed to 'pack'.
-        """
-        if self._write_group is not self.get_transaction():
-            # has an unlock or relock occured ?
-            raise errors.BzrError('mismatched lock context %r and '
-                'write group %r.' %
-                (self.get_transaction(), self._write_group))
-        result = self._commit_write_group()
-        self._write_group = None
-        return result
-
-    def _commit_write_group(self):
-        """Template method for per-repository write group cleanup.
-
-        This is called before the write group is considered to be
-        finished and should ensure that all data handed to the repository
-        for writing during the write group is safely committed (to the
-        extent possible considering file system caching etc).
-        """
-
-    def suspend_write_group(self):
-        raise errors.UnsuspendableWriteGroup(self)
-
-    def refresh_data(self):
-        """Re-read any data needed to synchronise with disk.
-
-        This method is intended to be called after another repository instance
-        (such as one used by a smart server) has inserted data into the
-        repository. On all repositories this will work outside of write groups.
-        Some repository formats (pack and newer for bzrlib native formats)
-        support refresh_data inside write groups. If called inside a write
-        group on a repository that does not support refreshing in a write group
-        IsInWriteGroupError will be raised.
-        """
-        self._refresh_data()
-
-    def resume_write_group(self, tokens):
-        if not self.is_write_locked():
-            raise errors.NotWriteLocked(self)
-        if self._write_group:
-            raise errors.BzrError('already in a write group')
-        self._resume_write_group(tokens)
-        # so we can detect unlock/relock - the write group is now entered.
-        self._write_group = self.get_transaction()
-
-    def _resume_write_group(self, tokens):
-        raise errors.UnsuspendableWriteGroup(self)
-
     def fetch(self, source, revision_id=None, find_ghosts=False,
             fetch_spec=None):
         """Fetch the content required to construct revision_id from source.
@@ -684,10 +573,6 @@
         If revision_id is None and fetch_spec is None, then all content is
         copied.
 
-        fetch() may not be used when the repository is in a write group -
-        either finish the current write group before using fetch, or use
-        fetch before starting the write group.
-
         :param find_ghosts: Find and copy revisions in the source that are
             ghosts in the target (and not reachable directly by walking out to
             the first-present revision in target from revision_id).
@@ -702,9 +587,6 @@
         if fetch_spec is not None and revision_id is not None:
             raise AssertionError(
                 "fetch_spec and revision_id are mutually exclusive.")
-        if self.is_in_write_group():
-            raise errors.InternalBzrError(
-                "May not fetch while in a write group.")
         # fast path same-url fetch operations
         # TODO: lift out to somewhere common with RemoteRepository
         # <https://bugs.launchpad.net/bzr/+bug/401646>
@@ -744,13 +626,6 @@
 
     @only_raises(errors.LockNotHeld, errors.LockBroken)
     def unlock(self):
-        if (self.control_files._lock_count == 1 and
-            self.control_files._lock_mode == 'w'):
-            if self._write_group is not None:
-                self.abort_write_group()
-                self.control_files.unlock()
-                raise errors.BzrError(
-                    'Must end write groups before releasing write locks.')
         self.control_files.unlock()
         if self.control_files._lock_count == 0:
             for repo in self._fallback_repositories:
@@ -771,35 +646,6 @@
         self.copy_content_into(dest_repo, revision_id)
         return dest_repo
 
-    def start_write_group(self):
-        """Start a write group in the repository.
-
-        Write groups are used by repositories which do not have a 1:1 mapping
-        between file ids and backend store to manage the insertion of data from
-        both fetch and commit operations.
-
-        A write lock is required around the start_write_group/commit_write_group
-        for the support of lock-requiring repository formats.
-
-        One can only insert data into a repository inside a write group.
-
-        :return: None.
-        """
-        if not self.is_write_locked():
-            raise errors.NotWriteLocked(self)
-        if self._write_group:
-            raise errors.BzrError('already in a write group')
-        self._start_write_group()
-        # so we can detect unlock/relock - the write group is now entered.
-        self._write_group = self.get_transaction()
-
-    def _start_write_group(self):
-        """Template method for per-repository write group startup.
-
-        This is called before the write group is considered to be
-        entered.
-        """
-
     @needs_read_lock
     def sprout(self, to_bzrdir, revision_id=None):
         """Create a descendent repository for new development.

=== modified file 'bzrlib/tests/per_repository/__init__.py'
--- bzrlib/tests/per_repository/__init__.py	2011-05-09 17:52:54 +0000
+++ bzrlib/tests/per_repository/__init__.py	2011-05-12 15:41:07 +0000
@@ -124,11 +124,9 @@
         'test_merge_directive',
         'test_pack',
         'test_reconcile',
-        'test_refresh_data',
         'test_repository',
         'test_revision',
         'test_statistics',
-        'test_write_group',
         ]
     # Parameterize per_repository test modules by format.
     submod_tests = loader.loadTestsFromModuleNames(

=== modified file 'bzrlib/tests/per_repository_vf/__init__.py'
--- bzrlib/tests/per_repository_vf/__init__.py	2011-05-09 17:52:54 +0000
+++ bzrlib/tests/per_repository_vf/__init__.py	2011-05-12 15:41:07 +0000
@@ -43,7 +43,9 @@
         'test__generate_text_key_index',
         'test_fetch',
         'test_reconcile',
+        'test_refresh_data',
         'test_repository',
+        'test_write_group',
         ]
     basic_tests.addTest(loader.loadTestsFromModuleNames(
             ["%s.%s" % (__name__, tmn) for tmn in testmod_names]))

=== renamed file 'bzrlib/tests/per_repository/test_refresh_data.py' => 'bzrlib/tests/per_repository_vf/test_refresh_data.py'
--- bzrlib/tests/per_repository/test_refresh_data.py	2010-05-11 14:14:55 +0000
+++ bzrlib/tests/per_repository_vf/test_refresh_data.py	2011-05-12 15:41:07 +0000
@@ -18,14 +18,23 @@
 
 from bzrlib import (
     errors,
-    repository,
+    vf_repository,
     )
+from bzrlib.tests.scenarios import load_tests_apply_scenarios
 from bzrlib.tests import TestSkipped
-from bzrlib.tests.per_repository import TestCaseWithRepository
+from bzrlib.tests.per_repository_vf import (
+    TestCaseWithRepository,
+    all_repository_vf_format_scenarios,
+    )
+
+
+load_tests = load_tests_apply_scenarios
 
 
 class TestRefreshData(TestCaseWithRepository):
 
+    scenarios = all_repository_vf_format_scenarios()
+
     def test_refresh_data_unlocked(self):
         # While not interesting, it should not error.
         repo = self.make_repository('.')
@@ -55,7 +64,7 @@
         self.addCleanup(repo.abort_write_group)
         try:
             repo.refresh_data()
-        except repository.IsInWriteGroupError:
+        except vf_repository.IsInWriteGroupError:
             # This is ok.
             pass
         else:
@@ -112,7 +121,7 @@
         # succeeds and the new revisions are visible.
         try:
             repo.refresh_data()
-        except repository.IsInWriteGroupError:
+        except vf_repository.IsInWriteGroupError:
             pass
         else:
             self.assertEqual(

=== renamed file 'bzrlib/tests/per_repository/test_write_group.py' => 'bzrlib/tests/per_repository_vf/test_write_group.py'
--- bzrlib/tests/per_repository/test_write_group.py	2011-01-10 23:52:14 +0000
+++ bzrlib/tests/per_repository_vf/test_write_group.py	2011-05-12 15:41:07 +0000
@@ -22,21 +22,28 @@
     branch,
     bzrdir,
     errors,
-    graph,
     memorytree,
-    osutils,
     remote,
     tests,
     versionedfile,
     )
 from bzrlib.tests import (
-    per_repository,
     test_server,
     )
+from bzrlib.tests.per_repository_vf import (
+    TestCaseWithRepository,
+    all_repository_vf_format_scenarios,
+    )
+from bzrlib.tests.scenarios import load_tests_apply_scenarios
 from bzrlib.transport import memory
 
 
-class TestWriteGroup(per_repository.TestCaseWithRepository):
+load_tests = load_tests_apply_scenarios
+
+
+class TestWriteGroup(TestCaseWithRepository):
+
+    scenarios = all_repository_vf_format_scenarios()
 
     def test_start_write_group_unlocked_needs_write_lock(self):
         repo = self.make_repository('.')
@@ -134,7 +141,9 @@
         self.assertEqual(None, repo.abort_write_group(suppress_errors=True))
 
 
-class TestGetMissingParentInventories(per_repository.TestCaseWithRepository):
+class TestGetMissingParentInventories(TestCaseWithRepository):
+
+    scenarios = all_repository_vf_format_scenarios()
 
     def test_empty_get_missing_parent_inventories(self):
         """A new write group has no missing parent inventories."""
@@ -393,7 +402,9 @@
 
 
 
-class TestResumeableWriteGroup(per_repository.TestCaseWithRepository):
+class TestResumeableWriteGroup(TestCaseWithRepository):
+
+    scenarios = all_repository_vf_format_scenarios()
 
     def make_write_locked_repo(self, relpath='repo'):
         repo = self.make_repository(relpath)

=== modified file 'bzrlib/vf_repository.py'
--- bzrlib/vf_repository.py	2011-05-08 13:06:20 +0000
+++ bzrlib/vf_repository.py	2011-05-12 15:41:07 +0000
@@ -66,10 +66,20 @@
     )
 
 from bzrlib.trace import (
+    log_exception_quietly,
     mutter,
+    note,
     )
 
 
+class IsInWriteGroupError(errors.InternalBzrError):
+
+    _fmt = "May not refresh_data of repo %(repo)s while in a write group."
+
+    def __init__(self, repo):
+        errors.InternalBzrError.__init__(self, repo=repo)
+
+
 class VersionedFileRepositoryFormat(RepositoryFormat):
     """Base class for all repository formats that are VersionedFiles-based."""
 
@@ -899,6 +909,138 @@
     # get_commit_builder.
     _commit_builder_class = VersionedFileCommitBuilder
 
+    def start_write_group(self):
+        """Start a write group in the repository.
+
+        Write groups are used by repositories which do not have a 1:1 mapping
+        between file ids and backend store to manage the insertion of data from
+        both fetch and commit operations.
+
+        A write lock is required around the start_write_group/commit_write_group
+        for the support of lock-requiring repository formats.
+
+        One can only insert data into a repository inside a write group.
+
+        :return: None.
+        """
+        if not self.is_write_locked():
+            raise errors.NotWriteLocked(self)
+        if self._write_group:
+            raise errors.BzrError('already in a write group')
+        self._start_write_group()
+        # so we can detect unlock/relock - the write group is now entered.
+        self._write_group = self.get_transaction()
+
+    def _start_write_group(self):
+        """Template method for per-repository write group startup.
+
+        This is called before the write group is considered to be
+        entered.
+        """
+
+    def abort_write_group(self, suppress_errors=False):
+        """Commit the contents accrued within the current write group.
+
+        :param suppress_errors: if true, abort_write_group will catch and log
+            unexpected errors that happen during the abort, rather than
+            allowing them to propagate.  Defaults to False.
+
+        :seealso: start_write_group.
+        """
+        if self._write_group is not self.get_transaction():
+            # has an unlock or relock occured ?
+            if suppress_errors:
+                mutter(
+                '(suppressed) mismatched lock context and write group. %r, %r',
+                self._write_group, self.get_transaction())
+                return
+            raise errors.BzrError(
+                'mismatched lock context and write group. %r, %r' %
+                (self._write_group, self.get_transaction()))
+        try:
+            self._abort_write_group()
+        except Exception, exc:
+            self._write_group = None
+            if not suppress_errors:
+                raise
+            mutter('abort_write_group failed')
+            log_exception_quietly()
+            note('bzr: ERROR (ignored): %s', exc)
+        self._write_group = None
+
+    def _abort_write_group(self):
+        """Template method for per-repository write group cleanup.
+
+        This is called during abort before the write group is considered to be
+        finished and should cleanup any internal state accrued during the write
+        group. There is no requirement that data handed to the repository be
+        *not* made available - this is not a rollback - but neither should any
+        attempt be made to ensure that data added is fully commited. Abort is
+        invoked when an error has occured so futher disk or network operations
+        may not be possible or may error and if possible should not be
+        attempted.
+        """
+
+    def is_in_write_group(self):
+        """Return True if there is an open write group.
+
+        :seealso: start_write_group.
+        """
+        return self._write_group is not None
+
+    def commit_write_group(self):
+        """Commit the contents accrued within the current write group.
+
+        :seealso: start_write_group.
+        
+        :return: it may return an opaque hint that can be passed to 'pack'.
+        """
+        if self._write_group is not self.get_transaction():
+            # has an unlock or relock occured ?
+            raise errors.BzrError('mismatched lock context %r and '
+                'write group %r.' %
+                (self.get_transaction(), self._write_group))
+        result = self._commit_write_group()
+        self._write_group = None
+        return result
+
+    def _commit_write_group(self):
+        """Template method for per-repository write group cleanup.
+
+        This is called before the write group is considered to be
+        finished and should ensure that all data handed to the repository
+        for writing during the write group is safely committed (to the
+        extent possible considering file system caching etc).
+        """
+
+    def suspend_write_group(self):
+        raise errors.UnsuspendableWriteGroup(self)
+
+    def resume_write_group(self, tokens):
+        if not self.is_write_locked():
+            raise errors.NotWriteLocked(self)
+        if self._write_group:
+            raise errors.BzrError('already in a write group')
+        self._resume_write_group(tokens)
+        # so we can detect unlock/relock - the write group is now entered.
+        self._write_group = self.get_transaction()
+
+    def _resume_write_group(self, tokens):
+        raise errors.UnsuspendableWriteGroup(self)
+
+    def refresh_data(self):
+        """Re-read any data needed to synchronise with disk.
+
+        This method is intended to be called after another repository instance
+        (such as one used by a smart server) has inserted data into the
+        repository. On all repositories this will work outside of write groups.
+        Some repository formats (pack and newer for bzrlib native formats)
+        support refresh_data inside write groups. If called inside a write
+        group on a repository that does not support refreshing in a write group
+        IsInWriteGroupError will be raised.
+        """
+        self._refresh_data()
+
     def add_fallback_repository(self, repository):
         """Add a repository to use for looking up data not held locally.
 
@@ -921,6 +1063,13 @@
 
     @only_raises(errors.LockNotHeld, errors.LockBroken)
     def unlock(self):
+        if (self.control_files._lock_count == 1 and
+            self.control_files._lock_mode == 'w'):
+            if self._write_group is not None:
+                self.abort_write_group()
+                self.control_files.unlock()
+                raise errors.BzrError(
+                    'Must end write groups before releasing write locks.')
         super(VersionedFileRepository, self).unlock()
         if self.control_files._lock_count == 0:
             self._inventory_entry_cache.clear()
@@ -1189,6 +1338,7 @@
         # Is it safe to return inventory entries directly from the entry cache,
         # rather copying them?
         self._safe_to_return_from_cache = False
+        self._write_group = None
 
     @needs_read_lock
     def gather_stats(self, revid=None, committers=None):
@@ -1204,6 +1354,20 @@
             # result['size'] = t
         return result
 
+    def fetch(self, source, revision_id=None, find_ghosts=False,
+            fetch_spec=None):
+        """See Repository.fetch().
+
+        fetch() may not be used when the repository is in a write group -
+        either finish the current write group before using fetch, or use
+        fetch before starting the write group.
+        """
+        if self.is_in_write_group():
+            raise errors.InternalBzrError(
+                "May not fetch while in a write group.")
+        return super(VersionedFileRepository, self).fetch(source,
+            revision_id=revision_id, find_ghosts=find_ghosts,
+            fetch_spec=fetch_spec)
 
     def get_commit_builder(self, branch, parents, config, timestamp=None,
                            timezone=None, committer=None, revprops=None,