Merge lp:~mbp/bzr/integration-old into lp:~bzr/bzr/trunk-old

[Martin Pool]

[Martin Pool (mbp)]

In the distant past we did a lot of file IO indirectly through LockableFiles. We now use the transport directly. This deletes some deprecated methods that forwarded to the transport, and their tests.

CountedLock.get_physical_lock status merely forwards so doesn't urgently need a test.

In passing I noticed this strange behaviour that get_branch_transport may or may not actually create a directory depending on whether you specify the format, thus the comment.

[John A Meinel (jameinel)]

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

Martin Pool wrote:
> Martin Pool has proposed merging lp:~mbp/bzr/integration into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
> In the distant past we did a lot of file IO indirectly through LockableFiles. We now use the transport directly. This deletes some deprecated methods that forwarded to the transport, and their tests.
>
> CountedLock.get_physical_lock status merely forwards so doesn't urgently need a test.
>
> In passing I noticed this strange behaviour that get_branch_transport may or may not actually create a directory depending on whether you specify the format, thus the comment.
>

 review: approve
 merge: approve

Though I'm a bit surprised you consider it difficult to test the
get_physical_lock_status.

Also, what is calling this function? As I see you added something new,
but did not add any new callers.

John
=:->

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

iEYEARECAAYFAkp26FEACgkQJdeBCYSNAANdtACeJt0Tkv0erqSnI6fQaA8G5IO6
l2cAoNM6YvsPiiejgSeS+U3nnef+SGEH
=88nM
-----END PGP SIGNATURE-----

review: Approve

[Martin Pool (mbp)]

2009/8/3 John A Meinel <email address hidden>:
> Though I'm a bit surprised you consider it difficult to test the
> get_physical_lock_status.

Not hard, I was just a bit lazy, and I kind of felt this should go
into some kind of parameterized test.

> Also, what is calling this function? As I see you added something new,
> but did not add any new callers.

Nothing yet; I'm working towards cleaning out the different aspects of
LockableFiles and this is one of them.

--
Martin <http://launchpad.net/~mbp/>

=== modified file 'bzrlib/bzrdir.py'
--- bzrlib/bzrdir.py	2009-08-03 04:11:06 +0000
+++ bzrlib/bzrdir.py	2009-08-04 14:35:47 +0000
@@ -1634,6 +1634,8 @@
 
     def get_branch_transport(self, branch_format):
         """See BzrDir.get_branch_transport()."""
+        # XXX: this shouldn't implicitly create the directory if it's just
+        # promising to get a transport -- mbp 20090727
         if branch_format is None:
             return self.transport.clone('branch')
         try:

=== modified file 'bzrlib/counted_lock.py'
--- bzrlib/counted_lock.py	2009-03-25 04:20:12 +0000
+++ bzrlib/counted_lock.py	2009-08-04 14:35:47 +0000
@@ -46,6 +46,18 @@
         self._lock_mode = None
         self._lock_count = 0
 
+    def get_physical_lock_status(self):
+        """Return physical lock status.
+
+        Returns true if a lock is held on the transport. If no lock is held, or
+        the underlying locking mechanism does not support querying lock
+        status, false is returned.
+        """
+        try:
+            return self._real_lock.peek() is not None
+        except NotImplementedError:
+            return False
+
     def is_locked(self):
         return self._lock_mode is not None
 

=== modified file 'bzrlib/lockable_files.py'
--- bzrlib/lockable_files.py	2009-07-10 07:47:21 +0000
+++ bzrlib/lockable_files.py	2009-08-04 14:35:47 +0000
@@ -65,11 +65,7 @@
 class LockableFiles(object):
     """Object representing a set of related files locked within the same scope.
 
-    These files are used by a WorkingTree, Repository or Branch, and should
-    generally only be touched by that object.
-
-    LockableFiles also provides some policy on top of Transport for encoding
-    control files as utf-8.
+    This coordinates access to the lock along with providing a transaction.
 
     LockableFiles manage a lock count and can be locked repeatedly by
     a single caller.  (The underlying lock implementation generally does not
@@ -77,13 +73,6 @@
 
     Instances of this class are often called control_files.
 
-    This object builds on top of a Transport, which is used to actually write
-    the files to disk, and an OSLock or LockDir, which controls how access to
-    the files is controlled.  The particular type of locking used is set when
-    the object is constructed.  In older formats OSLocks are used everywhere.
-    in newer formats a LockDir is used for Repositories and Branches, and
-    OSLocks for the local filesystem.
-
     This class is now deprecated; code should move to using the Transport
     directly for file operations and using the lock or CountedLock for
     locking.
@@ -151,7 +140,7 @@
     def _find_modes(self):
         """Determine the appropriate modes for files and directories.
 
-        :deprecated: Replaced by BzrDir._find_modes.
+        :deprecated: Replaced by BzrDir._find_creation_modes.
         """
         # XXX: The properties created by this can be removed or deprecated
         # once all the _get_text_store methods etc no longer use them.
@@ -170,81 +159,6 @@
             # Remove the sticky and execute bits for files
             self._file_mode = self._dir_mode & ~07111
 
-    @deprecated_method(deprecated_in((1, 6, 0)))
-    def controlfilename(self, file_or_path):
-        """Return location relative to branch.
-
-        :deprecated: Use Transport methods instead.
-        """
-        return self._transport.abspath(self._escape(file_or_path))
-
-    @needs_read_lock
-    @deprecated_method(deprecated_in((1, 5, 0)))
-    def get(self, relpath):
-        """Get a file as a bytestream.
-
-        :deprecated: Use a Transport instead of LockableFiles.
-        """
-        relpath = self._escape(relpath)
-        return self._transport.get(relpath)
-
-    @needs_read_lock
-    @deprecated_method(deprecated_in((1, 5, 0)))
-    def get_utf8(self, relpath):
-        """Get a file as a unicode stream.
-
-        :deprecated: Use a Transport instead of LockableFiles.
-        """
-        relpath = self._escape(relpath)
-        # DO NOT introduce an errors=replace here.
-        return codecs.getreader('utf-8')(self._transport.get(relpath))
-
-    @needs_write_lock
-    @deprecated_method(deprecated_in((1, 6, 0)))
-    def put(self, path, file):
-        """Write a file.
-
-        :param path: The path to put the file, relative to the .bzr control
-                     directory
-        :param file: A file-like or string object whose contents should be copied.
-
-        :deprecated: Use Transport methods instead.
-        """
-        self._transport.put_file(self._escape(path), file, mode=self._file_mode)
-
-    @needs_write_lock
-    @deprecated_method(deprecated_in((1, 6, 0)))
-    def put_bytes(self, path, a_string):
-        """Write a string of bytes.
-
-        :param path: The path to put the bytes, relative to the transport root.
-        :param a_string: A string object, whose exact bytes are to be copied.
-
-        :deprecated: Use Transport methods instead.
-        """
-        self._transport.put_bytes(self._escape(path), a_string,
-                                  mode=self._file_mode)
-
-    @needs_write_lock
-    @deprecated_method(deprecated_in((1, 6, 0)))
-    def put_utf8(self, path, a_string):
-        """Write a string, encoding as utf-8.
-
-        :param path: The path to put the string, relative to the transport root.
-        :param string: A string or unicode object whose contents should be copied.
-
-        :deprecated: Use Transport methods instead.
-        """
-        # IterableFile would not be needed if Transport.put took iterables
-        # instead of files.  ADHB 2005-12-25
-        # RBC 20060103 surely its not needed anyway, with codecs transcode
-        # file support ?
-        # JAM 20060103 We definitely don't want encode(..., 'replace')
-        # these are valuable files which should have exact contents.
-        if not isinstance(a_string, basestring):
-            raise errors.BzrBadParameterNotString(a_string)
-        self.put_bytes(path, a_string.encode('utf-8'))
-
     def leave_in_place(self):
         """Set this LockableFiles to not clear the physical lock on unlock."""
         self._lock.leave_in_place()

=== modified file 'bzrlib/tests/test_counted_lock.py'
--- bzrlib/tests/test_counted_lock.py	2009-03-25 04:20:12 +0000
+++ bzrlib/tests/test_counted_lock.py	2009-08-04 14:35:47 +0000
@@ -215,3 +215,5 @@
         l.break_lock()
         self.assertFalse(l.is_locked())
         self.assertFalse(real_lock.is_locked())
+
+    # TODO: test get_physical_lock_status

=== modified file 'bzrlib/tests/test_lockable_files.py'
--- bzrlib/tests/test_lockable_files.py	2009-06-23 08:43:05 +0000
+++ bzrlib/tests/test_lockable_files.py	2009-08-04 14:35:47 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2005, 2006, 2008 Canonical Ltd
+# Copyright (C) 2005, 2006, 2008, 2009 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -46,79 +46,6 @@
 # so won't modernize them now. - mbp 20080430
 class _TestLockableFiles_mixin(object):
 
-    def test_read_write(self):
-        self.assertRaises(NoSuchFile,
-            self.applyDeprecated,
-            deprecated_in((1, 5, 0)),
-            self.lockable.get, 'foo')
-        self.assertRaises(NoSuchFile,
-            self.applyDeprecated,
-            deprecated_in((1, 5, 0)),
-            self.lockable.get_utf8, 'foo')
-        self.lockable.lock_write()
-        self.addCleanup(self.lockable.unlock)
-        unicode_string = u'bar\u1234'
-        self.assertEqual(4, len(unicode_string))
-        byte_string = unicode_string.encode('utf-8')
-        self.assertEqual(6, len(byte_string))
-        self.assertRaises(UnicodeEncodeError,
-            self.applyDeprecated,
-            deprecated_in((1, 6, 0)),
-            self.lockable.put, 'foo',
-            StringIO(unicode_string))
-        self.applyDeprecated(
-            deprecated_in((1, 6, 0)),
-            self.lockable.put,
-            'foo', StringIO(byte_string))
-        byte_stream = self.applyDeprecated(
-            deprecated_in((1, 5, 0)),
-            self.lockable.get,
-            'foo')
-        self.assertEqual(byte_string, byte_stream.read())
-        unicode_stream = self.applyDeprecated(
-            deprecated_in((1, 5, 0)),
-            self.lockable.get_utf8,
-            'foo')
-        self.assertEqual(unicode_string,
-            unicode_stream.read())
-        self.assertRaises(BzrBadParameterNotString,
-            self.applyDeprecated,
-            deprecated_in((1, 6, 0)),
-            self.lockable.put_utf8,
-            'bar',
-            StringIO(unicode_string))
-        self.applyDeprecated(
-            deprecated_in((1, 6, 0)),
-            self.lockable.put_utf8,
-            'bar',
-            unicode_string)
-        unicode_stream = self.applyDeprecated(
-            deprecated_in((1, 5, 0)),
-            self.lockable.get_utf8,
-            'bar')
-        self.assertEqual(unicode_string,
-            unicode_stream.read())
-        byte_stream = self.applyDeprecated(
-            deprecated_in((1, 5, 0)),
-            self.lockable.get,
-            'bar')
-        self.assertEqual(byte_string, byte_stream.read())
-        self.applyDeprecated(
-            deprecated_in((1, 6, 0)),
-            self.lockable.put_bytes,
-            'raw', 'raw\xffbytes')
-        byte_stream = self.applyDeprecated(
-            deprecated_in((1, 5, 0)),
-            self.lockable.get,
-            'raw')
-        self.assertEqual('raw\xffbytes', byte_stream.read())
-
-    def test_locks(self):
-        self.lockable.lock_read()
-        self.addCleanup(self.lockable.unlock)
-        self.assertRaises(ReadOnlyError, self.lockable.put, 'foo',
-                          StringIO('bar\u1234'))
-
     def test_transactions(self):
         self.assertIs(self.lockable.get_transaction().__class__,
                       PassThroughTransaction)