[patch] storage refactoring ready to merge?

John Arbash Meinel john at arbash-meinel.com
Thu Jan 12 15:32:17 GMT 2006


Martin Pool wrote:
> Here's an updated version of the "storage" branch, which many people
> have had a hand in by now.  I've reviewed it today and made some small
> changes and will merge it someone else would like to contribute a +1.
> 
> The changes (most from other people) include:
> 
>  * add LockableFiles grouping
> 
>  * store history in a Repository object rather than in the Branch itself
> 
>  * remove bzrlib.clone
> 
>  * split @needs_read_lock, @needs_write_lock into bzrlib.decorators
> 
>  * add IterableFile
> 
>  * remove many interfaces from Branch that don't (or no longer) 
>    properly belong there
> 
> 

To start with, I think the storage changes are very nice. My biggest
concern is that Repository, WorkingTree, and Branch are all going to
lock the same file. Which is valid under Linux, but not Windows. I
describe it more later.


> === added file 'bzrlib/decorators.py'
> --- /dev/null	
> +++ bzrlib/decorators.py	
> @@ -0,0 +1,49 @@
> +# Copyright (C) 2005 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
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> +
> +
> +def needs_read_lock(unbound):
> +    """Decorate unbound to take out and release a read lock.
> +
> +    This decorator can be applied to methods of any class with lock_read() and
> +    unlock() methods.
> +    
> +    Typical usage:
> +        
> +    class Branch(...):
> +        @needs_read_lock
> +        def branch_method(self, ...):
> +            stuff
> +    """
> +    def decorated(self, *args, **kwargs):
> +        self.lock_read()
> +        try:
> +            return unbound(self, *args, **kwargs)
> +        finally:
> +            self.unlock()
> +    return decorated

Can we get decorators with at least a minimal amount of copying.
+    def decorated(self, *args, **kwargs):
+        self.lock_read()
+        try:
+            return unbound(self, *args, **kwargs)
+        finally:
+            self.unlock()
+    decorated.__name__ = unbound.__name__
+    decorated.__doc__ = unbound.__doc__
+    return decorated

Eventually we would want to create a _copy_decorated_info() function.
Which Robert has written most of. It just makes online help() more useful.

This doesn't prevent merging, but it would nice to have at least a TODO.

> === added file 'bzrlib/iterablefile.py'

...

> +
> +    def read_n(self, length):
> +        """
> +        >>> IterableFileBase(['This ', 'is ', 'a ', 'test.']).read_n(8)
> +        'This is '
> +        """
> +        def test_length(result):
> +            if len(result) >= length:
> +                return length
> +            else:
> +                return None
> +        return self._read(test_length)

...

General comment about IterableFile, it is designed to be tested with
DOCTEST, but I don't see an addition to bzrlib/tests/__init__.py
MODULES_TO_DOCTEST.

...

> +
> +    def lock_write(self):
> +        mutter("lock write: %s (%s)", self, self._lock_count)
> +        # TODO: Upgrade locking to support using a Transport,
> +        # and potentially a remote locking protocol
> +        if self._lock_mode:
> +            if self._lock_mode != 'w':
> +                raise LockError("can't upgrade to a write lock from %r" %
> +                                self._lock_mode)
> +            self._lock_count += 1
> +        else:
> +            self._lock = self._transport.lock_write(
> +                    self._escape(self.lock_name))
> +            self._lock_mode = 'w'
> +            self._lock_count = 1
> +            self._set_transaction(transactions.PassThroughTransaction())
> +

We still have the problem that WorkingTree is going to lock the same
file that Branch is going to lock (as is Repository).
On Windows, locking the same file more than once causes a deadlock. They
all keep different counters, so they will lock/unlock at their own time.
Not even necessarily in order (though with try/finally they probably
will all be correctly nested).
Anyway, if we are going to have different code re-using the same lock
file, we need to share a Lock object somehow.
(This is the same problem as doing the 'bzr pull .' which causes a
deadlock on windows)

In branch.py:

> +    def peek_lock_mode(self):
> +        """Return lock mode for the Branch: 'r', 'w' or None"""
> +        raise NotImplementedError(self.is_locked)
> +

Should be raise NotImplementedError(self.peek_lock_mode)

...
>  ######################################################################
>  # branch objects
> @@ -144,7 +126,7 @@
>      @staticmethod
>      def initialize(base):
>          """Create a new branch, rooted at 'base' (url)"""
> -        t = get_transport(base)
> +        t = get_transport(unicode(base))
>          return BzrBranch(t, init=True)

This is a decent enough workaround for now. But ultimately, we aren't
supposed to be passing unicode objects to Transports. They are supposed
to only get URLs. Which means passing
t = get_transport(urllib.quote(unicode(base).encode('utf-8')))

LocalTransport definitely needs a lot of work to only give and receive
URLs. (list_dir() needs to urllib.quote() each entry, etc).

This isn't specifically related to the storage changes, though.

>  
>      def setup_caching(self, cache_root):

Is setup_caching ever used or even called? Or is it dead code that
should be removed.

...

>  
> +    def clone(self, to_location, revision=None, basis_branch=None, to_branch_type=None):
> +        """Copy this branch into the existing directory to_location.
> +
> +        Returns the newly created branch object.
> +
> +        revision
> +            If not None, only revisions up to this point will be copied.
> +            The head of the new branch will be that revision.  Must be a
> +            revid or None.
> +    
> +        to_location -- The destination directory; must either exist and be 
> +            empty, or not exist, in which case it is created.
> +    
> +        basis_branch
> +            A local branch to copy revisions from, related to this branch. 
> +            This is used when branching from a remote (slow) branch, and we have
> +            a local branch that might contain some relevant revisions.
> +    
> +        to_branch_type
> +            Branch type of destination branch
> +        """
> +        assert isinstance(to_location, basestring)
> +        if not bzrlib.osutils.lexists(to_location):
> +            os.mkdir(to_location)
> +        if to_branch_type is None:
> +            to_branch_type = BzrBranch
> +        br_to = to_branch_type.initialize(to_location)
> +        mutter("copy branch from %s to %s", self, br_to)
> +        if basis_branch is not None:
> +            basis_branch.push_stores(br_to)
> +        br_to.working_tree().set_root_id(self.get_root_id())
> +        if revision is None:
> +            revision = self.last_revision()
> +        br_to.update_revisions(self, stop_revision=revision)
> +        br_to.set_parent(self.base)
> +        # circular import protection
> +        from bzrlib.merge import build_working_dir
> +        build_working_dir(to_location)
> +        mutter("copied")
> +        return br_to
> +

# circular import protection
is probably a bogus comment. And if we are going to import in a
function, we should do it at the beginning of the function.

...

This was a long patch to review completely line-by-line. But I tried. In
general I'm +1 on the changes, and would like to see this land shortly
after 0.7 is released, after the locking issue is fixed. (I would hate
to have an official 0.7 release which didn't work on windows.)

John
=:->


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 256 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060112/abda7cbb/attachment.pgp 


More information about the bazaar mailing list