Bazaar

Merge lp:~jameinel/bzr/2.0.4-unregister-mem-trans into lp:bzr/2.0

  1. 2.0.4-unregister-mem-trans
  2. Merge into 2.0
Proposed by John A Meinel
Status: Rejected
Rejected by: John A Meinel
Proposed branch: lp:~jameinel/bzr/2.0.4-unregister-mem-trans
Merge into: lp:bzr/2.0
Diff against target: 269 lines (+54/-32)
3 files modified
bzrlib/tests/__init__.py (+1/-1)
bzrlib/tests/test_transport.py (+45/-28)
bzrlib/transport/memory.py (+8/-3)
To merge this branch: bzr merge lp:~jameinel/bzr/2.0.4-unregister-mem-trans
Reviewer Review Type Date Requested Status
John A Meinel Disapprove
Martin Pool Disapprove
Andrew Bennetts Approve
Review via email: mp+16980@code.launchpad.net
To post a comment you must log in.
Revision history for this message
John A Meinel (jameinel) wrote :

Martin noticed that MemoryServer doesn't unregister itself. This adds that to tearDown, and adds some tests for it.

In a quick test, I did see a bunch of MemoryServer registrations left over at the end of the test suite. For example, running all of "per_pack_repository" (300 tests) left about 84 registered memory transports. From what I can tell it doesn't actually leave the MemoryServer in memory, just some functions registered in the transport_list_registry. However, it is hygienic to clean them out.

Revision history for this message
Andrew Bennetts (spiv) wrote :

Looks good to me.

I wonder a little if the risk/reward tradeoff is right for lp:bzr/2.0, but it's probably fine. Definitely good to have in trunk, of course.

review: Approve
Revision history for this message
Martin Pool (mbp) wrote :

I would not merge this to 2.0 at least without further discussion: it doesn't specifically fix serious bugs, it is fairly large and it has some risk.

I hope John just misclicked when proposing the merge.

review: Disapprove
Revision history for this message
John A Meinel (jameinel) wrote :

There are a fair amount of mechanical changes (move MemoryTransport => memory.MemoryTransport), but the bulk of the change is just:
255 @@ -315,11 +318,13 @@
256 result._files = self._files
257 result._locks = self._locks
258 return result
259 - register_transport(self._scheme, memory_factory)
260 + self._memory_factory = memory_factory
261 + register_transport(self._scheme, self._memory_factory)
262
263 def tearDown(self):
264 """See bzrlib.transport.Server.tearDown."""
265 # unregister this server
266 + unregister_transport(self._scheme, self._memory_factory)
267
268 def get_url(self):
269 """See bzrlib.transport.Server.get_url."""

Which I feel is sufficiently small to warrant merging to 2.0. If Martin feels differently, I'm fine only merging this to 2.1.

Revision history for this message
Martin Pool (mbp) wrote :

I think the main thing here is

* it doesn't actually benefit 2.0 users
* it makes the diff bigger for people to read in the next SRU
* any code change, even mechanically, has some small risk

I tried to explain this more in https://code.edge.launchpad.net/~mbp/bzr/doc/+merge/16997

Revision history for this message
John A Meinel (jameinel) wrote :

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

Martin Pool wrote:
> I think the main thing here is
>
> * it doesn't actually benefit 2.0 users
> * it makes the diff bigger for people to read in the next SRU
> * any code change, even mechanically, has some small risk
>
> I tried to explain this more in https://code.edge.launchpad.net/~mbp/bzr/doc/+merge/16997

I had forgotten about the need to have a bug for SRU purposes.

Generally, I try to target anything small to 2.0, because otherwise my
default behavior is to just do everything in 'dev' and then nothing gets
into 2.0.

I'll just land this in 2.1.

 merge: rejected

John
=:->

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

iEYEARECAAYFAktHXqUACgkQJdeBCYSNAAM7HwCeJ0kQ4ybnsxLbO7JItlnJXyv5
n8MAnRzNCbfTBRUsUowPGO4w6EwovP0v
=XA3T
-----END PGP SIGNATURE-----

review: Disapprove

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/tests/__init__.py'
2--- bzrlib/tests/__init__.py 2009-10-29 04:19:13 +0000
3+++ bzrlib/tests/__init__.py 2010-01-07 18:19:12 +0000
4@@ -1,4 +1,4 @@
5-# Copyright (C) 2005, 2006, 2007, 2008, 2009 Canonical Ltd
6+# Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010 Canonical Ltd
7 #
8 # This program is free software; you can redistribute it and/or modify
9 # it under the terms of the GNU General Public License as published by
10
11=== modified file 'bzrlib/tests/test_transport.py'
12--- bzrlib/tests/test_transport.py 2009-03-23 14:59:43 +0000
13+++ bzrlib/tests/test_transport.py 2010-01-07 18:19:12 +0000
14@@ -1,4 +1,4 @@
15-# Copyright (C) 2004, 2005, 2006, 2007 Canonical Ltd
16+# Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010 Canonical Ltd
17 #
18 # This program is free software; you can redistribute it and/or modify
19 # it under the terms of the GNU General Public License as published by
20@@ -17,12 +17,15 @@
21
22 from cStringIO import StringIO
23
24-import bzrlib
25 from bzrlib import (
26 errors,
27 osutils,
28+ transport as _mod_transport,
29 urlutils,
30 )
31+from bzrlib.transport import (
32+ memory,
33+ )
34 from bzrlib.errors import (DependencyNotPresent,
35 FileExists,
36 InvalidURLJoin,
37@@ -45,7 +48,6 @@
38 Transport,
39 )
40 from bzrlib.transport.chroot import ChrootServer
41-from bzrlib.transport.memory import MemoryTransport
42 from bzrlib.transport.local import (LocalTransport,
43 EmulatedWin32LocalTransport)
44
45@@ -159,7 +161,7 @@
46
47 def test_local_abspath_non_local_transport(self):
48 # the base implementation should throw
49- t = MemoryTransport()
50+ t = memory.MemoryTransport()
51 e = self.assertRaises(errors.NotLocalUrl, t.local_abspath, 't')
52 self.assertEqual('memory:///t is not a local path.', str(e))
53
54@@ -249,68 +251,83 @@
55 max_size=1*1024*1024*1024)
56
57
58+class TestMemoryServer(TestCase):
59+
60+ def test_create_server(self):
61+ server = memory.MemoryServer()
62+ server.setUp()
63+ url = server.get_url()
64+ self.assertTrue(url in _mod_transport.transport_list_registry)
65+ t = _mod_transport.get_transport(url)
66+ del t
67+ server.tearDown()
68+ self.assertFalse(url in _mod_transport.transport_list_registry)
69+ self.assertRaises(errors.UnsupportedProtocol,
70+ _mod_transport.get_transport, url)
71+
72+
73 class TestMemoryTransport(TestCase):
74
75 def test_get_transport(self):
76- MemoryTransport()
77+ memory.MemoryTransport()
78
79 def test_clone(self):
80- transport = MemoryTransport()
81- self.assertTrue(isinstance(transport, MemoryTransport))
82+ transport = memory.MemoryTransport()
83+ self.assertTrue(isinstance(transport, memory.MemoryTransport))
84 self.assertEqual("memory:///", transport.clone("/").base)
85
86 def test_abspath(self):
87- transport = MemoryTransport()
88+ transport = memory.MemoryTransport()
89 self.assertEqual("memory:///relpath", transport.abspath('relpath'))
90
91 def test_abspath_of_root(self):
92- transport = MemoryTransport()
93+ transport = memory.MemoryTransport()
94 self.assertEqual("memory:///", transport.base)
95 self.assertEqual("memory:///", transport.abspath('/'))
96
97 def test_abspath_of_relpath_starting_at_root(self):
98- transport = MemoryTransport()
99+ transport = memory.MemoryTransport()
100 self.assertEqual("memory:///foo", transport.abspath('/foo'))
101
102 def test_append_and_get(self):
103- transport = MemoryTransport()
104+ transport = memory.MemoryTransport()
105 transport.append_bytes('path', 'content')
106 self.assertEqual(transport.get('path').read(), 'content')
107 transport.append_file('path', StringIO('content'))
108 self.assertEqual(transport.get('path').read(), 'contentcontent')
109
110 def test_put_and_get(self):
111- transport = MemoryTransport()
112+ transport = memory.MemoryTransport()
113 transport.put_file('path', StringIO('content'))
114 self.assertEqual(transport.get('path').read(), 'content')
115 transport.put_bytes('path', 'content')
116 self.assertEqual(transport.get('path').read(), 'content')
117
118 def test_append_without_dir_fails(self):
119- transport = MemoryTransport()
120+ transport = memory.MemoryTransport()
121 self.assertRaises(NoSuchFile,
122 transport.append_bytes, 'dir/path', 'content')
123
124 def test_put_without_dir_fails(self):
125- transport = MemoryTransport()
126+ transport = memory.MemoryTransport()
127 self.assertRaises(NoSuchFile,
128 transport.put_file, 'dir/path', StringIO('content'))
129
130 def test_get_missing(self):
131- transport = MemoryTransport()
132+ transport = memory.MemoryTransport()
133 self.assertRaises(NoSuchFile, transport.get, 'foo')
134
135 def test_has_missing(self):
136- transport = MemoryTransport()
137+ transport = memory.MemoryTransport()
138 self.assertEquals(False, transport.has('foo'))
139
140 def test_has_present(self):
141- transport = MemoryTransport()
142+ transport = memory.MemoryTransport()
143 transport.append_bytes('foo', 'content')
144 self.assertEquals(True, transport.has('foo'))
145
146 def test_list_dir(self):
147- transport = MemoryTransport()
148+ transport = memory.MemoryTransport()
149 transport.put_bytes('foo', 'content')
150 transport.mkdir('dir')
151 transport.put_bytes('dir/subfoo', 'content')
152@@ -320,28 +337,28 @@
153 self.assertEquals(['subfoo'], sorted(transport.list_dir('dir')))
154
155 def test_mkdir(self):
156- transport = MemoryTransport()
157+ transport = memory.MemoryTransport()
158 transport.mkdir('dir')
159 transport.append_bytes('dir/path', 'content')
160 self.assertEqual(transport.get('dir/path').read(), 'content')
161
162 def test_mkdir_missing_parent(self):
163- transport = MemoryTransport()
164+ transport = memory.MemoryTransport()
165 self.assertRaises(NoSuchFile,
166 transport.mkdir, 'dir/dir')
167
168 def test_mkdir_twice(self):
169- transport = MemoryTransport()
170+ transport = memory.MemoryTransport()
171 transport.mkdir('dir')
172 self.assertRaises(FileExists, transport.mkdir, 'dir')
173
174 def test_parameters(self):
175- transport = MemoryTransport()
176+ transport = memory.MemoryTransport()
177 self.assertEqual(True, transport.listable())
178 self.assertEqual(False, transport.is_readonly())
179
180 def test_iter_files_recursive(self):
181- transport = MemoryTransport()
182+ transport = memory.MemoryTransport()
183 transport.mkdir('dir')
184 transport.put_bytes('dir/foo', 'content')
185 transport.put_bytes('dir/bar', 'content')
186@@ -350,7 +367,7 @@
187 self.assertEqual(set(['dir/foo', 'dir/bar', 'bar']), paths)
188
189 def test_stat(self):
190- transport = MemoryTransport()
191+ transport = memory.MemoryTransport()
192 transport.put_bytes('foo', 'content')
193 transport.put_bytes('bar', 'phowar')
194 self.assertEqual(7, transport.stat('foo').st_size)
195@@ -420,25 +437,25 @@
196 class ChrootServerTest(TestCase):
197
198 def test_construct(self):
199- backing_transport = MemoryTransport()
200+ backing_transport = memory.MemoryTransport()
201 server = ChrootServer(backing_transport)
202 self.assertEqual(backing_transport, server.backing_transport)
203
204 def test_setUp(self):
205- backing_transport = MemoryTransport()
206+ backing_transport = memory.MemoryTransport()
207 server = ChrootServer(backing_transport)
208 server.setUp()
209 self.assertTrue(server.scheme in _get_protocol_handlers().keys())
210
211 def test_tearDown(self):
212- backing_transport = MemoryTransport()
213+ backing_transport = memory.MemoryTransport()
214 server = ChrootServer(backing_transport)
215 server.setUp()
216 server.tearDown()
217 self.assertFalse(server.scheme in _get_protocol_handlers().keys())
218
219 def test_get_url(self):
220- backing_transport = MemoryTransport()
221+ backing_transport = memory.MemoryTransport()
222 server = ChrootServer(backing_transport)
223 server.setUp()
224 self.assertEqual('chroot-%d:///' % id(server), server.get_url())
225
226=== modified file 'bzrlib/transport/memory.py'
227--- bzrlib/transport/memory.py 2009-07-20 04:26:55 +0000
228+++ bzrlib/transport/memory.py 2010-01-07 18:19:12 +0000
229@@ -1,4 +1,4 @@
230-# Copyright (C) 2005, 2006 Canonical Ltd
231+# Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010 Canonical Ltd
232 #
233 # This program is free software; you can redistribute it and/or modify
234 # it under the terms of the GNU General Public License as published by
235@@ -27,6 +27,9 @@
236 from cStringIO import StringIO
237 import warnings
238
239+from bzrlib import (
240+ urlutils,
241+ )
242 from bzrlib.errors import (
243 FileExists,
244 LockError,
245@@ -42,8 +45,8 @@
246 register_transport,
247 Server,
248 Transport,
249+ unregister_transport,
250 )
251-import bzrlib.urlutils as urlutils
252
253
254
255@@ -315,11 +318,13 @@
256 result._files = self._files
257 result._locks = self._locks
258 return result
259- register_transport(self._scheme, memory_factory)
260+ self._memory_factory = memory_factory
261+ register_transport(self._scheme, self._memory_factory)
262
263 def tearDown(self):
264 """See bzrlib.transport.Server.tearDown."""
265 # unregister this server
266+ unregister_transport(self._scheme, self._memory_factory)
267
268 def get_url(self):
269 """See bzrlib.transport.Server.get_url."""

Subscribers

People subscribed via source and target branches