Merge lp:~lifeless/bzr/loomsupport into lp:bzr

[Robert Collins]

A lazy-load support glue for Branch formats.

This is an alternative implementation of lazy format objects. It seemed
simpler to me than another custom registry subclass (needed to handle the
network name stuff properly), and this permits slightly cleaner backwards
compatability code in loom than otherwise.

[lp:~lifeless/bzr/loomsupport]

5306. By Canonical.com Patch Queue Manager <email address hidden> on 2010-06-18

test_import_tariff should respect BZR_PLUGINS_AT and BZR_DISABLE_PLUGINS

5307. By Canonical.com Patch Queue Manager <email address hidden> on 2010-06-18

Clarify documentation about pqm usage.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Robert Collins wrote:
> Robert Collins has proposed merging lp:~lifeless/bzr/loomsupport into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
>
> This is an alternative implementation of lazy format objects. It seemed
> simpler to me than another custom registry subclass (needed to handle the
> network name stuff properly), and this permits slightly cleaner backwards
> compatability code in loom than otherwise.
>

+ # registered as factories.
+ if isinstance(format, MetaDirBranchFormatFactory):
+
network_format_registry.register(format.get_format_string(), format)
+ else:
+ network_format_registry.register(format.get_format_string(),
+ format.__class__)

^- This looks a bit odd.

I believe I understand why you are doing it, but it really makes me wonder.

Why are we passing in instances if what we want is factories...

I think if we add this, we should register at least a couple formats
lazily, so that people have examples to pick from.

Right now, the only example is in the test suite, and it is particularly
bad, because it uses "Class().get_format_string()" which certainly isn't
how you would use it in production. (If you had an instance of the
class, you would just register it directly...)

 review: needsfixing
 merge: approve

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkwbgMoACgkQJdeBCYSNAAN1qgCgm1mof+1ZMB20NZu+b+jTDpHn
D2QAniOCiIaL9OZ+XR0X7zWyv/ISo5sw
=k8Nv
-----END PGP SIGNATURE-----

review: Needs Fixing

[Robert Collins (lifeless)]

loom uses it - could I point at the loom use as a reference?

I mean, I'd love to move the old branch formats to be lazy too, but
that seems non trivial.

[Robert Collins (lifeless)]

As there wasn't any followup to my query, I'm going to land this pretty much as is. I'll change the test suite example to use a constant string, and add a pointer to loom.

[Robert Collins (lifeless)]

sent to pqm by email

[lp:~lifeless/bzr/loomsupport]

5308. By Canonical.com Patch Queue Manager <email address hidden> on 2010-06-20

(lifeless) A lazy-load support glue for Branch formats. (Robert Collins)

=== modified file 'NEWS'
--- NEWS	2010-06-18 11:57:13 +0000
+++ NEWS	2010-06-20 21:19:24 +0000
@@ -92,6 +92,12 @@
   plugins to intercept this even when a ``RemoteBranch`` proxy is in use.
   (Robert Collins, #201613)
 
+* ``Branch`` formats can now be loaded lazily by registering a
+  ``MetaDirBranchFormatFactory`` rather than an actual format. This will
+  cause the named format class to be loaded only when an enumeration of
+  formats is needed or when the format string for the object is
+  encountered. (Robert Collins, Jelmer Vernooij)
+
 * Use lazy imports in ``bzrlib/merge.py`` so that plugins like ``news_merge``
   do not cause modules to be loaded unnecessarily just because the plugin
   registers a merge hook.  This improves ``bzr rocks`` time by about 25%

=== modified file 'bzrlib/branch.py'
--- bzrlib/branch.py	2010-06-17 06:30:22 +0000
+++ bzrlib/branch.py	2010-06-20 21:19:24 +0000
@@ -1518,7 +1518,10 @@
         try:
             transport = a_bzrdir.get_branch_transport(None, name=name)
             format_string = transport.get_bytes("format")
-            return klass._formats[format_string]
+            format = klass._formats[format_string]
+            if isinstance(format, MetaDirBranchFormatFactory):
+                return format()
+            return format
         except errors.NoSuchFile:
             raise errors.NotBranchError(path=transport.base, bzrdir=a_bzrdir)
         except KeyError:
@@ -1529,6 +1532,20 @@
         """Return the current default format."""
         return klass._default_format
 
+    @classmethod
+    def get_formats(klass):
+        """Get all the known formats.
+
+        Warning: This triggers a load of all lazy registered formats: do not
+        use except when that is desireed.
+        """
+        result = []
+        for fmt in klass._formats.values():
+            if isinstance(fmt, MetaDirBranchFormatFactory):
+                fmt = fmt()
+            result.append(fmt)
+        return result
+
     def get_reference(self, a_bzrdir, name=None):
         """Get the target reference of the branch in a_bzrdir.
 
@@ -1671,11 +1688,19 @@
 
     @classmethod
     def register_format(klass, format):
-        """Register a metadir format."""
+        """Register a metadir format.
+        
+        See MetaDirBranchFormatFactory for the ability to register a format
+        without loading the code the format needs until it is actually used.
+        """
         klass._formats[format.get_format_string()] = format
         # Metadir formats have a network name of their format string, and get
-        # registered as class factories.
-        network_format_registry.register(format.get_format_string(), format.__class__)
+        # registered as factories.
+        if isinstance(format, MetaDirBranchFormatFactory):
+            network_format_registry.register(format.get_format_string(), format)
+        else:
+            network_format_registry.register(format.get_format_string(),
+                format.__class__)
 
     @classmethod
     def set_default_format(klass, format):
@@ -1701,6 +1726,34 @@
         return False  # by default
 
 
+class MetaDirBranchFormatFactory(registry._LazyObjectGetter):
+    """A factory for a BranchFormat object, permitting simple lazy registration.
+    
+    While none of the built in BranchFormats are lazy registered yet,
+    bzrlib.tests.test_branch.TestMetaDirBranchFormatFactory demonstrates how to
+    use it, and the bzr-loom plugin uses it as well (see
+    bzrlib.plugins.loom.formats).
+    """
+
+    def __init__(self, format_string, module_name, member_name):
+        """Create a MetaDirBranchFormatFactory.
+
+        :param format_string: The format string the format has.
+        :param module_name: Module to load the format class from.
+        :param member_name: Attribute name within the module for the format class.
+        """
+        registry._LazyObjectGetter.__init__(self, module_name, member_name)
+        self._format_string = format_string
+        
+    def get_format_string(self):
+        """See BranchFormat.get_format_string."""
+        return self._format_string
+
+    def __call__(self):
+        """Used for network_format_registry support."""
+        return self.get_obj()()
+
+
 class BranchHooks(Hooks):
     """A dictionary mapping hook name to a list of callables for branch hooks.
 

=== modified file 'bzrlib/tests/per_branch/__init__.py'
--- bzrlib/tests/per_branch/__init__.py	2010-05-13 15:14:41 +0000
+++ bzrlib/tests/per_branch/__init__.py	2010-06-20 21:19:24 +0000
@@ -132,7 +132,7 @@
     # Generate a list of branch formats and their associated bzrdir formats to
     # use.
     combinations = [(format, format._matchingbzrdir) for format in
-         BranchFormat._formats.values() + _legacy_formats]
+         BranchFormat.get_formats() + _legacy_formats]
     scenarios = make_scenarios(
         # None here will cause the default vfs transport server to be used.
         None,

=== modified file 'bzrlib/tests/test_branch.py'
--- bzrlib/tests/test_branch.py	2010-04-23 07:15:23 +0000
+++ bzrlib/tests/test_branch.py	2010-06-20 21:19:24 +0000
@@ -136,6 +136,27 @@
         return "opened branch."
 
 
+# Demonstrating how lazy loading is often implemented:
+# A constant string is created.
+SampleSupportedBranchFormatString = "Sample supported branch format."
+
+# And the format class can then reference the constant to avoid skew.
+class SampleSupportedBranchFormat(_mod_branch.BranchFormat):
+    """A sample supported format."""
+
+    def get_format_string(self):
+        """See BzrBranchFormat.get_format_string()."""
+        return SampleSupportedBranchFormatString
+
+    def initialize(self, a_bzrdir, name=None):
+        t = a_bzrdir.get_branch_transport(self, name=name)
+        t.put_bytes('format', self.get_format_string())
+        return 'A branch'
+
+    def open(self, transport, name=None, _found=False, ignore_fallbacks=False):
+        return "opened supported branch."
+
+
 class TestBzrBranchFormat(tests.TestCaseWithTransport):
     """Tests for the BzrBranchFormat facility."""
 
@@ -152,6 +173,17 @@
             self.failUnless(isinstance(found_format, format.__class__))
         check_format(_mod_branch.BzrBranchFormat5(), "bar")
 
+    def test_find_format_factory(self):
+        dir = bzrdir.BzrDirMetaFormat1().initialize(self.get_url())
+        SampleSupportedBranchFormat().initialize(dir)
+        factory = _mod_branch.MetaDirBranchFormatFactory(
+            SampleSupportedBranchFormatString,
+            "bzrlib.tests.test_branch", "SampleSupportedBranchFormat")
+        _mod_branch.BranchFormat.register_format(factory)
+        self.addCleanup(_mod_branch.BranchFormat.unregister_format, factory)
+        b = _mod_branch.Branch.open(self.get_url())
+        self.assertEqual(b, "opened supported branch.")
+
     def test_find_format_not_branch(self):
         dir = bzrdir.BzrDirMetaFormat1().initialize(self.get_url())
         self.assertRaises(errors.NotBranchError,
@@ -186,6 +218,34 @@
         self.make_branch_and_tree('bar')
 
 
+#Used by TestMetaDirBranchFormatFactory 
+FakeLazyFormat = None
+
+
+class TestMetaDirBranchFormatFactory(tests.TestCase):
+
+    def test_get_format_string_does_not_load(self):
+        """Formats have a static format string."""
+        factory = _mod_branch.MetaDirBranchFormatFactory("yo", None, None)
+        self.assertEqual("yo", factory.get_format_string())
+
+    def test_call_loads(self):
+        # __call__ is used by the network_format_registry interface to get a
+        # Format.
+        global FakeLazyFormat
+        del FakeLazyFormat
+        factory = _mod_branch.MetaDirBranchFormatFactory(None,
+            "bzrlib.tests.test_branch", "FakeLazyFormat")
+        self.assertRaises(AttributeError, factory)
+
+    def test_call_returns_call_of_referenced_object(self):
+        global FakeLazyFormat
+        FakeLazyFormat = lambda:'called'
+        factory = _mod_branch.MetaDirBranchFormatFactory(None,
+            "bzrlib.tests.test_branch", "FakeLazyFormat")
+        self.assertEqual('called', factory())
+
+
 class TestBranch67(object):
     """Common tests for both branch 6 and 7 which are mostly the same."""