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

Proposed by Robert Collins
Status: Merged
Approved by: John A Meinel
Approved revision: no longer in the source branch.
Merged at revision: 5308
Proposed branch: lp:~lifeless/bzr/loomsupport
Merge into: lp:bzr
Diff against target: 208 lines (+124/-5)
4 files modified
NEWS (+6/-0)
bzrlib/branch.py (+57/-4)
bzrlib/tests/per_branch/__init__.py (+1/-1)
bzrlib/tests/test_branch.py (+60/-0)
To merge this branch: bzr merge lp:~lifeless/bzr/loomsupport
Reviewer Review Type Date Requested Status
John A Meinel Needs Fixing
Review via email: mp+27903@code.launchpad.net

Commit message

A lazy-load support glue for Branch formats.

Description of the change

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.

To post a comment you must log in.
Revision history for this message
John A Meinel (jameinel) wrote :

-----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
Revision history for this message
Robert Collins (lifeless) wrote :

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.

Revision history for this message
Robert Collins (lifeless) wrote :

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.

Revision history for this message
Robert Collins (lifeless) wrote :

sent to pqm by email

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2010-06-18 11:57:13 +0000
3+++ NEWS 2010-06-20 21:19:24 +0000
4@@ -92,6 +92,12 @@
5 plugins to intercept this even when a ``RemoteBranch`` proxy is in use.
6 (Robert Collins, #201613)
7
8+* ``Branch`` formats can now be loaded lazily by registering a
9+ ``MetaDirBranchFormatFactory`` rather than an actual format. This will
10+ cause the named format class to be loaded only when an enumeration of
11+ formats is needed or when the format string for the object is
12+ encountered. (Robert Collins, Jelmer Vernooij)
13+
14 * Use lazy imports in ``bzrlib/merge.py`` so that plugins like ``news_merge``
15 do not cause modules to be loaded unnecessarily just because the plugin
16 registers a merge hook. This improves ``bzr rocks`` time by about 25%
17
18=== modified file 'bzrlib/branch.py'
19--- bzrlib/branch.py 2010-06-17 06:30:22 +0000
20+++ bzrlib/branch.py 2010-06-20 21:19:24 +0000
21@@ -1518,7 +1518,10 @@
22 try:
23 transport = a_bzrdir.get_branch_transport(None, name=name)
24 format_string = transport.get_bytes("format")
25- return klass._formats[format_string]
26+ format = klass._formats[format_string]
27+ if isinstance(format, MetaDirBranchFormatFactory):
28+ return format()
29+ return format
30 except errors.NoSuchFile:
31 raise errors.NotBranchError(path=transport.base, bzrdir=a_bzrdir)
32 except KeyError:
33@@ -1529,6 +1532,20 @@
34 """Return the current default format."""
35 return klass._default_format
36
37+ @classmethod
38+ def get_formats(klass):
39+ """Get all the known formats.
40+
41+ Warning: This triggers a load of all lazy registered formats: do not
42+ use except when that is desireed.
43+ """
44+ result = []
45+ for fmt in klass._formats.values():
46+ if isinstance(fmt, MetaDirBranchFormatFactory):
47+ fmt = fmt()
48+ result.append(fmt)
49+ return result
50+
51 def get_reference(self, a_bzrdir, name=None):
52 """Get the target reference of the branch in a_bzrdir.
53
54@@ -1671,11 +1688,19 @@
55
56 @classmethod
57 def register_format(klass, format):
58- """Register a metadir format."""
59+ """Register a metadir format.
60+
61+ See MetaDirBranchFormatFactory for the ability to register a format
62+ without loading the code the format needs until it is actually used.
63+ """
64 klass._formats[format.get_format_string()] = format
65 # Metadir formats have a network name of their format string, and get
66- # registered as class factories.
67- network_format_registry.register(format.get_format_string(), format.__class__)
68+ # registered as factories.
69+ if isinstance(format, MetaDirBranchFormatFactory):
70+ network_format_registry.register(format.get_format_string(), format)
71+ else:
72+ network_format_registry.register(format.get_format_string(),
73+ format.__class__)
74
75 @classmethod
76 def set_default_format(klass, format):
77@@ -1701,6 +1726,34 @@
78 return False # by default
79
80
81+class MetaDirBranchFormatFactory(registry._LazyObjectGetter):
82+ """A factory for a BranchFormat object, permitting simple lazy registration.
83+
84+ While none of the built in BranchFormats are lazy registered yet,
85+ bzrlib.tests.test_branch.TestMetaDirBranchFormatFactory demonstrates how to
86+ use it, and the bzr-loom plugin uses it as well (see
87+ bzrlib.plugins.loom.formats).
88+ """
89+
90+ def __init__(self, format_string, module_name, member_name):
91+ """Create a MetaDirBranchFormatFactory.
92+
93+ :param format_string: The format string the format has.
94+ :param module_name: Module to load the format class from.
95+ :param member_name: Attribute name within the module for the format class.
96+ """
97+ registry._LazyObjectGetter.__init__(self, module_name, member_name)
98+ self._format_string = format_string
99+
100+ def get_format_string(self):
101+ """See BranchFormat.get_format_string."""
102+ return self._format_string
103+
104+ def __call__(self):
105+ """Used for network_format_registry support."""
106+ return self.get_obj()()
107+
108+
109 class BranchHooks(Hooks):
110 """A dictionary mapping hook name to a list of callables for branch hooks.
111
112
113=== modified file 'bzrlib/tests/per_branch/__init__.py'
114--- bzrlib/tests/per_branch/__init__.py 2010-05-13 15:14:41 +0000
115+++ bzrlib/tests/per_branch/__init__.py 2010-06-20 21:19:24 +0000
116@@ -132,7 +132,7 @@
117 # Generate a list of branch formats and their associated bzrdir formats to
118 # use.
119 combinations = [(format, format._matchingbzrdir) for format in
120- BranchFormat._formats.values() + _legacy_formats]
121+ BranchFormat.get_formats() + _legacy_formats]
122 scenarios = make_scenarios(
123 # None here will cause the default vfs transport server to be used.
124 None,
125
126=== modified file 'bzrlib/tests/test_branch.py'
127--- bzrlib/tests/test_branch.py 2010-04-23 07:15:23 +0000
128+++ bzrlib/tests/test_branch.py 2010-06-20 21:19:24 +0000
129@@ -136,6 +136,27 @@
130 return "opened branch."
131
132
133+# Demonstrating how lazy loading is often implemented:
134+# A constant string is created.
135+SampleSupportedBranchFormatString = "Sample supported branch format."
136+
137+# And the format class can then reference the constant to avoid skew.
138+class SampleSupportedBranchFormat(_mod_branch.BranchFormat):
139+ """A sample supported format."""
140+
141+ def get_format_string(self):
142+ """See BzrBranchFormat.get_format_string()."""
143+ return SampleSupportedBranchFormatString
144+
145+ def initialize(self, a_bzrdir, name=None):
146+ t = a_bzrdir.get_branch_transport(self, name=name)
147+ t.put_bytes('format', self.get_format_string())
148+ return 'A branch'
149+
150+ def open(self, transport, name=None, _found=False, ignore_fallbacks=False):
151+ return "opened supported branch."
152+
153+
154 class TestBzrBranchFormat(tests.TestCaseWithTransport):
155 """Tests for the BzrBranchFormat facility."""
156
157@@ -152,6 +173,17 @@
158 self.failUnless(isinstance(found_format, format.__class__))
159 check_format(_mod_branch.BzrBranchFormat5(), "bar")
160
161+ def test_find_format_factory(self):
162+ dir = bzrdir.BzrDirMetaFormat1().initialize(self.get_url())
163+ SampleSupportedBranchFormat().initialize(dir)
164+ factory = _mod_branch.MetaDirBranchFormatFactory(
165+ SampleSupportedBranchFormatString,
166+ "bzrlib.tests.test_branch", "SampleSupportedBranchFormat")
167+ _mod_branch.BranchFormat.register_format(factory)
168+ self.addCleanup(_mod_branch.BranchFormat.unregister_format, factory)
169+ b = _mod_branch.Branch.open(self.get_url())
170+ self.assertEqual(b, "opened supported branch.")
171+
172 def test_find_format_not_branch(self):
173 dir = bzrdir.BzrDirMetaFormat1().initialize(self.get_url())
174 self.assertRaises(errors.NotBranchError,
175@@ -186,6 +218,34 @@
176 self.make_branch_and_tree('bar')
177
178
179+#Used by TestMetaDirBranchFormatFactory
180+FakeLazyFormat = None
181+
182+
183+class TestMetaDirBranchFormatFactory(tests.TestCase):
184+
185+ def test_get_format_string_does_not_load(self):
186+ """Formats have a static format string."""
187+ factory = _mod_branch.MetaDirBranchFormatFactory("yo", None, None)
188+ self.assertEqual("yo", factory.get_format_string())
189+
190+ def test_call_loads(self):
191+ # __call__ is used by the network_format_registry interface to get a
192+ # Format.
193+ global FakeLazyFormat
194+ del FakeLazyFormat
195+ factory = _mod_branch.MetaDirBranchFormatFactory(None,
196+ "bzrlib.tests.test_branch", "FakeLazyFormat")
197+ self.assertRaises(AttributeError, factory)
198+
199+ def test_call_returns_call_of_referenced_object(self):
200+ global FakeLazyFormat
201+ FakeLazyFormat = lambda:'called'
202+ factory = _mod_branch.MetaDirBranchFormatFactory(None,
203+ "bzrlib.tests.test_branch", "FakeLazyFormat")
204+ self.assertEqual('called', factory())
205+
206+
207 class TestBranch67(object):
208 """Common tests for both branch 6 and 7 which are mostly the same."""
209