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
=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py 2009-10-29 04:19:13 +0000
+++ bzrlib/tests/__init__.py 2010-01-07 18:19:12 +0000
@@ -1,4 +1,4 @@
1# Copyright (C) 2005, 2006, 2007, 2008, 2009 Canonical Ltd1# Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010 Canonical Ltd
2#2#
3# This program is free software; you can redistribute it and/or modify3# This program is free software; you can redistribute it and/or modify
4# it under the terms of the GNU General Public License as published by4# it under the terms of the GNU General Public License as published by
55
=== modified file 'bzrlib/tests/test_transport.py'
--- bzrlib/tests/test_transport.py 2009-03-23 14:59:43 +0000
+++ bzrlib/tests/test_transport.py 2010-01-07 18:19:12 +0000
@@ -1,4 +1,4 @@
1# Copyright (C) 2004, 2005, 2006, 2007 Canonical Ltd1# Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010 Canonical Ltd
2#2#
3# This program is free software; you can redistribute it and/or modify3# This program is free software; you can redistribute it and/or modify
4# it under the terms of the GNU General Public License as published by4# it under the terms of the GNU General Public License as published by
@@ -17,12 +17,15 @@
1717
18from cStringIO import StringIO18from cStringIO import StringIO
1919
20import bzrlib
21from bzrlib import (20from bzrlib import (
22 errors,21 errors,
23 osutils,22 osutils,
23 transport as _mod_transport,
24 urlutils,24 urlutils,
25 )25 )
26from bzrlib.transport import (
27 memory,
28 )
26from bzrlib.errors import (DependencyNotPresent,29from bzrlib.errors import (DependencyNotPresent,
27 FileExists,30 FileExists,
28 InvalidURLJoin,31 InvalidURLJoin,
@@ -45,7 +48,6 @@
45 Transport,48 Transport,
46 )49 )
47from bzrlib.transport.chroot import ChrootServer50from bzrlib.transport.chroot import ChrootServer
48from bzrlib.transport.memory import MemoryTransport
49from bzrlib.transport.local import (LocalTransport,51from bzrlib.transport.local import (LocalTransport,
50 EmulatedWin32LocalTransport)52 EmulatedWin32LocalTransport)
5153
@@ -159,7 +161,7 @@
159161
160 def test_local_abspath_non_local_transport(self):162 def test_local_abspath_non_local_transport(self):
161 # the base implementation should throw163 # the base implementation should throw
162 t = MemoryTransport()164 t = memory.MemoryTransport()
163 e = self.assertRaises(errors.NotLocalUrl, t.local_abspath, 't')165 e = self.assertRaises(errors.NotLocalUrl, t.local_abspath, 't')
164 self.assertEqual('memory:///t is not a local path.', str(e))166 self.assertEqual('memory:///t is not a local path.', str(e))
165167
@@ -249,68 +251,83 @@
249 max_size=1*1024*1024*1024)251 max_size=1*1024*1024*1024)
250252
251253
254class TestMemoryServer(TestCase):
255
256 def test_create_server(self):
257 server = memory.MemoryServer()
258 server.setUp()
259 url = server.get_url()
260 self.assertTrue(url in _mod_transport.transport_list_registry)
261 t = _mod_transport.get_transport(url)
262 del t
263 server.tearDown()
264 self.assertFalse(url in _mod_transport.transport_list_registry)
265 self.assertRaises(errors.UnsupportedProtocol,
266 _mod_transport.get_transport, url)
267
268
252class TestMemoryTransport(TestCase):269class TestMemoryTransport(TestCase):
253270
254 def test_get_transport(self):271 def test_get_transport(self):
255 MemoryTransport()272 memory.MemoryTransport()
256273
257 def test_clone(self):274 def test_clone(self):
258 transport = MemoryTransport()275 transport = memory.MemoryTransport()
259 self.assertTrue(isinstance(transport, MemoryTransport))276 self.assertTrue(isinstance(transport, memory.MemoryTransport))
260 self.assertEqual("memory:///", transport.clone("/").base)277 self.assertEqual("memory:///", transport.clone("/").base)
261278
262 def test_abspath(self):279 def test_abspath(self):
263 transport = MemoryTransport()280 transport = memory.MemoryTransport()
264 self.assertEqual("memory:///relpath", transport.abspath('relpath'))281 self.assertEqual("memory:///relpath", transport.abspath('relpath'))
265282
266 def test_abspath_of_root(self):283 def test_abspath_of_root(self):
267 transport = MemoryTransport()284 transport = memory.MemoryTransport()
268 self.assertEqual("memory:///", transport.base)285 self.assertEqual("memory:///", transport.base)
269 self.assertEqual("memory:///", transport.abspath('/'))286 self.assertEqual("memory:///", transport.abspath('/'))
270287
271 def test_abspath_of_relpath_starting_at_root(self):288 def test_abspath_of_relpath_starting_at_root(self):
272 transport = MemoryTransport()289 transport = memory.MemoryTransport()
273 self.assertEqual("memory:///foo", transport.abspath('/foo'))290 self.assertEqual("memory:///foo", transport.abspath('/foo'))
274291
275 def test_append_and_get(self):292 def test_append_and_get(self):
276 transport = MemoryTransport()293 transport = memory.MemoryTransport()
277 transport.append_bytes('path', 'content')294 transport.append_bytes('path', 'content')
278 self.assertEqual(transport.get('path').read(), 'content')295 self.assertEqual(transport.get('path').read(), 'content')
279 transport.append_file('path', StringIO('content'))296 transport.append_file('path', StringIO('content'))
280 self.assertEqual(transport.get('path').read(), 'contentcontent')297 self.assertEqual(transport.get('path').read(), 'contentcontent')
281298
282 def test_put_and_get(self):299 def test_put_and_get(self):
283 transport = MemoryTransport()300 transport = memory.MemoryTransport()
284 transport.put_file('path', StringIO('content'))301 transport.put_file('path', StringIO('content'))
285 self.assertEqual(transport.get('path').read(), 'content')302 self.assertEqual(transport.get('path').read(), 'content')
286 transport.put_bytes('path', 'content')303 transport.put_bytes('path', 'content')
287 self.assertEqual(transport.get('path').read(), 'content')304 self.assertEqual(transport.get('path').read(), 'content')
288305
289 def test_append_without_dir_fails(self):306 def test_append_without_dir_fails(self):
290 transport = MemoryTransport()307 transport = memory.MemoryTransport()
291 self.assertRaises(NoSuchFile,308 self.assertRaises(NoSuchFile,
292 transport.append_bytes, 'dir/path', 'content')309 transport.append_bytes, 'dir/path', 'content')
293310
294 def test_put_without_dir_fails(self):311 def test_put_without_dir_fails(self):
295 transport = MemoryTransport()312 transport = memory.MemoryTransport()
296 self.assertRaises(NoSuchFile,313 self.assertRaises(NoSuchFile,
297 transport.put_file, 'dir/path', StringIO('content'))314 transport.put_file, 'dir/path', StringIO('content'))
298315
299 def test_get_missing(self):316 def test_get_missing(self):
300 transport = MemoryTransport()317 transport = memory.MemoryTransport()
301 self.assertRaises(NoSuchFile, transport.get, 'foo')318 self.assertRaises(NoSuchFile, transport.get, 'foo')
302319
303 def test_has_missing(self):320 def test_has_missing(self):
304 transport = MemoryTransport()321 transport = memory.MemoryTransport()
305 self.assertEquals(False, transport.has('foo'))322 self.assertEquals(False, transport.has('foo'))
306323
307 def test_has_present(self):324 def test_has_present(self):
308 transport = MemoryTransport()325 transport = memory.MemoryTransport()
309 transport.append_bytes('foo', 'content')326 transport.append_bytes('foo', 'content')
310 self.assertEquals(True, transport.has('foo'))327 self.assertEquals(True, transport.has('foo'))
311328
312 def test_list_dir(self):329 def test_list_dir(self):
313 transport = MemoryTransport()330 transport = memory.MemoryTransport()
314 transport.put_bytes('foo', 'content')331 transport.put_bytes('foo', 'content')
315 transport.mkdir('dir')332 transport.mkdir('dir')
316 transport.put_bytes('dir/subfoo', 'content')333 transport.put_bytes('dir/subfoo', 'content')
@@ -320,28 +337,28 @@
320 self.assertEquals(['subfoo'], sorted(transport.list_dir('dir')))337 self.assertEquals(['subfoo'], sorted(transport.list_dir('dir')))
321338
322 def test_mkdir(self):339 def test_mkdir(self):
323 transport = MemoryTransport()340 transport = memory.MemoryTransport()
324 transport.mkdir('dir')341 transport.mkdir('dir')
325 transport.append_bytes('dir/path', 'content')342 transport.append_bytes('dir/path', 'content')
326 self.assertEqual(transport.get('dir/path').read(), 'content')343 self.assertEqual(transport.get('dir/path').read(), 'content')
327344
328 def test_mkdir_missing_parent(self):345 def test_mkdir_missing_parent(self):
329 transport = MemoryTransport()346 transport = memory.MemoryTransport()
330 self.assertRaises(NoSuchFile,347 self.assertRaises(NoSuchFile,
331 transport.mkdir, 'dir/dir')348 transport.mkdir, 'dir/dir')
332349
333 def test_mkdir_twice(self):350 def test_mkdir_twice(self):
334 transport = MemoryTransport()351 transport = memory.MemoryTransport()
335 transport.mkdir('dir')352 transport.mkdir('dir')
336 self.assertRaises(FileExists, transport.mkdir, 'dir')353 self.assertRaises(FileExists, transport.mkdir, 'dir')
337354
338 def test_parameters(self):355 def test_parameters(self):
339 transport = MemoryTransport()356 transport = memory.MemoryTransport()
340 self.assertEqual(True, transport.listable())357 self.assertEqual(True, transport.listable())
341 self.assertEqual(False, transport.is_readonly())358 self.assertEqual(False, transport.is_readonly())
342359
343 def test_iter_files_recursive(self):360 def test_iter_files_recursive(self):
344 transport = MemoryTransport()361 transport = memory.MemoryTransport()
345 transport.mkdir('dir')362 transport.mkdir('dir')
346 transport.put_bytes('dir/foo', 'content')363 transport.put_bytes('dir/foo', 'content')
347 transport.put_bytes('dir/bar', 'content')364 transport.put_bytes('dir/bar', 'content')
@@ -350,7 +367,7 @@
350 self.assertEqual(set(['dir/foo', 'dir/bar', 'bar']), paths)367 self.assertEqual(set(['dir/foo', 'dir/bar', 'bar']), paths)
351368
352 def test_stat(self):369 def test_stat(self):
353 transport = MemoryTransport()370 transport = memory.MemoryTransport()
354 transport.put_bytes('foo', 'content')371 transport.put_bytes('foo', 'content')
355 transport.put_bytes('bar', 'phowar')372 transport.put_bytes('bar', 'phowar')
356 self.assertEqual(7, transport.stat('foo').st_size)373 self.assertEqual(7, transport.stat('foo').st_size)
@@ -420,25 +437,25 @@
420class ChrootServerTest(TestCase):437class ChrootServerTest(TestCase):
421438
422 def test_construct(self):439 def test_construct(self):
423 backing_transport = MemoryTransport()440 backing_transport = memory.MemoryTransport()
424 server = ChrootServer(backing_transport)441 server = ChrootServer(backing_transport)
425 self.assertEqual(backing_transport, server.backing_transport)442 self.assertEqual(backing_transport, server.backing_transport)
426443
427 def test_setUp(self):444 def test_setUp(self):
428 backing_transport = MemoryTransport()445 backing_transport = memory.MemoryTransport()
429 server = ChrootServer(backing_transport)446 server = ChrootServer(backing_transport)
430 server.setUp()447 server.setUp()
431 self.assertTrue(server.scheme in _get_protocol_handlers().keys())448 self.assertTrue(server.scheme in _get_protocol_handlers().keys())
432449
433 def test_tearDown(self):450 def test_tearDown(self):
434 backing_transport = MemoryTransport()451 backing_transport = memory.MemoryTransport()
435 server = ChrootServer(backing_transport)452 server = ChrootServer(backing_transport)
436 server.setUp()453 server.setUp()
437 server.tearDown()454 server.tearDown()
438 self.assertFalse(server.scheme in _get_protocol_handlers().keys())455 self.assertFalse(server.scheme in _get_protocol_handlers().keys())
439456
440 def test_get_url(self):457 def test_get_url(self):
441 backing_transport = MemoryTransport()458 backing_transport = memory.MemoryTransport()
442 server = ChrootServer(backing_transport)459 server = ChrootServer(backing_transport)
443 server.setUp()460 server.setUp()
444 self.assertEqual('chroot-%d:///' % id(server), server.get_url())461 self.assertEqual('chroot-%d:///' % id(server), server.get_url())
445462
=== modified file 'bzrlib/transport/memory.py'
--- bzrlib/transport/memory.py 2009-07-20 04:26:55 +0000
+++ bzrlib/transport/memory.py 2010-01-07 18:19:12 +0000
@@ -1,4 +1,4 @@
1# Copyright (C) 2005, 2006 Canonical Ltd1# Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010 Canonical Ltd
2#2#
3# This program is free software; you can redistribute it and/or modify3# This program is free software; you can redistribute it and/or modify
4# it under the terms of the GNU General Public License as published by4# it under the terms of the GNU General Public License as published by
@@ -27,6 +27,9 @@
27from cStringIO import StringIO27from cStringIO import StringIO
28import warnings28import warnings
2929
30from bzrlib import (
31 urlutils,
32 )
30from bzrlib.errors import (33from bzrlib.errors import (
31 FileExists,34 FileExists,
32 LockError,35 LockError,
@@ -42,8 +45,8 @@
42 register_transport,45 register_transport,
43 Server,46 Server,
44 Transport,47 Transport,
48 unregister_transport,
45 )49 )
46import bzrlib.urlutils as urlutils
4750
4851
4952
@@ -315,11 +318,13 @@
315 result._files = self._files318 result._files = self._files
316 result._locks = self._locks319 result._locks = self._locks
317 return result320 return result
318 register_transport(self._scheme, memory_factory)321 self._memory_factory = memory_factory
322 register_transport(self._scheme, self._memory_factory)
319323
320 def tearDown(self):324 def tearDown(self):
321 """See bzrlib.transport.Server.tearDown."""325 """See bzrlib.transport.Server.tearDown."""
322 # unregister this server326 # unregister this server
327 unregister_transport(self._scheme, self._memory_factory)
323328
324 def get_url(self):329 def get_url(self):
325 """See bzrlib.transport.Server.get_url."""330 """See bzrlib.transport.Server.get_url."""

Subscribers

People subscribed via source and target branches