[patch] storage refactoring ready to merge?

[Martin Pool]

On 12 Jan 2006, John Arbash Meinel <john at arbash-meinel.com> wrote:

> 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.

Yes, done.

> 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.

Fixed now.

> 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)

I take it from Aaron's comments that this isn't actually a problem?

> 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)

Thanks, fixed.

> >      def setup_caching(self, cache_root):
> 
> Is setup_caching ever used or even called? Or is it dead code that
> should be removed.

I've made it just raise a warning for now.  Does anyone still want it?

> # 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.

Thanks, fixed.

> 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.)

It was an enormous patch; thanks very much for reviewing it.

-- 
Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060113/927375c4/attachment.pgp