[MERGE] Makeing WorkingTree nicer to derive from.

[Robert Collins]

On Fri, 2006-07-14 at 09:15 -0500, John Arbash Meinel wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Robert Collins wrote:
> > This patch:
> >  * factors workingtree.unlock to be cleaner
> >  * Adds a trivial test for the unlock method.
> >  * Moves unlock for Format2 trees into a new Format2 specific class, 
> >    making the base WorkingTree cleaner to derive from.
> > 
> 
> ...
> 
> v--- I thought we were doing:
> # Copyright (C) Canonical Ltd
> 
> And not doing Authors:
> 
> I also recently added 'test_locking.py' which already tests lock and
> unlock (and that they are in the proper order)
> 
> So I think this test is redundant.

True. I've nuked that new file. FWIW it predated our changes to
process. 


> >  class WorkingTree3(WorkingTree):
> >      """This is the Format 3 working tree.
> >  
> > @@ -1542,6 +1541,15 @@
> >              raise ConflictFormatError()
> >          return ConflictList.from_stanzas(RioReader(confile))
> >  
> > +    def unlock(self):
> > +        if self._hashcache.needs_write and self._control_files._lock_count==1:
> > +            self._hashcache.write()
> > +        # reverse order of locking.
> > +        try:
> > +            return self._control_files.unlock()
> > +        finally:
> > +            self.branch.unlock()
> > +
> 
> I like this refactoring, as it does make things cleaner. The only part
> here, is that 'self._hashcache.write()' needs a try/except around it, in
> the case that we have a readonly working tree.

I'm happy to add a TODO to that, but its certainly not in the pipeline
for what I'm working on - which is dirstate. dirstate will remove the
separate hashcache anyway, so there is no motivation to fix that as part
of this branch.
..
> Rather than putting a try/except around self._hashcache.write() twice,
> it might be better to have a helper function in the base working tree.

or perhaps write() should fail softly.

> Your refactoring is still +1 to come in. But I wouldn't mind discussing
> whether not being able to write the hashcache is a warning/note/or just
> a mutter.
> 
> The specific use case is running 'bzr status' or 'bzr info' on a
> readonly tree. (bzr diff doesn't seem to update the hashcache in my
> testing).


I looked at the existing lock unlock tests, and have a few key comments
about them. Firstly, here is a replacement patch for them, to make them
only test the interface, rather than breaking on other branch types -
such as 'hg' or 'git' branches.

There is one thing that was being tested that my replacement does not
test. This is the precise lock and unlock order of the lock() and
unlock() calls. Now, why am I not testing that anymore? I dont think
that the order a branch and tree pair decide to cooperate on the lock
and unlock sequence is an appropriate part of the interface. Its
possibly something to test in a implementation specific manner - which
your somewhat complex decorator approach did, but definately not
something to test that every implementation does. Interestingly, my
replacement tests exposed a bug - something implicitly tested but not
explicitly tested, which is that opening a format 6 branch twice in a
single python process would allow data destruction due to allowing both
instances to hold a write lock at once. This patch corrects that. This
is something that is tested as a result of using a second copy of the
branch to force lock failures.

Anyhow, would like your feedback on this.

Cheers,
Rob

-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: workingtree-unlock.patch
Type: text/x-patch
Size: 15505 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060716/f48bb53f/attachment.bin 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 191 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060716/f48bb53f/attachment.pgp