[MERGE] Makeing WorkingTree nicer to derive from.

[Martin Pool]

On 16 Jul 2006, Robert Collins <robertc at robertcollins.net> wrote:

Just some detail-oriented comments while we're still discussing it.

> +    def test_unlock_branch_failures(self):
> +        """If the branch unlock fails the tree must still unlock."""
> +        # The public interface for WorkingTree requires a branch, but
> +        # does not require that the working tree use the branch - its
> +        # implementation specific how the WorkingTree, Branch, and Repository
> +        # hang together.
> +        # in order to test that implementations which *do* unlock via the branch
> +        # do so correctly, we unlock the branch after locking the working tree.
> +        # The next unlock on working tree should trigger a LockNotHeld exception
> +        # from the branch object, which must be exposed to the caller. To meet 
> +        # our object model - where locking a tree locks its branch, and
> +        # unlocking a branch does not unlock a working tree, *even* for 
> +        # all-in-one implementations like bzr 0.6, git, and hg, implementations
> +        # must have some separate counter for each object, so our explicit 
> +        # unlock should trigger some error on all implementations, and 
> +        # requiring that to be LockNotHeld seems reasonable.
> +        #
> +        # we use this approach rather than decorating the Branch, because the
> +        # public interface of WorkingTree does not permit altering the branch
> +        # object - and we cannot tell which attribute might allow us to 
> +        # backdoor-in and change it reliably. For implementation specific tests
> +        # we can do such skullduggery, but not for interface specific tests.
> +        # And, its simpler :)
> +        wt = self.make_branch_and_tree('.')
> +
> +        self.assertFalse(wt.is_locked())
> +        self.assertFalse(wt.branch.is_locked())
> +        wt.lock_write()
> +        self.assertTrue(wt.is_locked())
> +        self.assertTrue(wt.branch.is_locked())
> +

> +        # manually unlock the branch, preparing a LockNotHeld error.
> +        wt.branch.unlock()
> +        # the branch *may* still be locked here, if its an all-in-one
> +        # implementation
> +        self.assertRaises(errors.LockNotHeld, wt.unlock)

Uh, so if it's still held it won't raise the exception, will it

> +        # but now, the tree must be unlocked
> +        self.assertFalse(wt.is_locked())
> +        # and the branch too.
> +        self.assertFalse(wt.branch.is_locked())
> +

> +    def test_failing_to_lock_branch_does_not_lock(self):
> +        """If the branch cannot be locked, dont lock the tree."""

I think you can afford to say _does_not_lock_tree, and similarly below. :-)

> +        # Many implementations treat read-locks as non-blocking, but some
> +        # treat them as blocking with writes.. Accordingly we test this by
> +        # opening the branch twice, and locking the branch for write in the
> +        # second instance.  Our lock contract requires separate instances to
> +        # mutually exclude if a lock is exclusive at all: If we get no error
> +        # locking, the test still passes.
> +        wt = self.make_branch_and_tree('.')
> +        branch_copy = branch.Branch.open('.')
> +        branch_copy.lock_write()
> +        try:
> +            try:
> +                wt.lock_read()
> +            except:
> +                # any error here means the locks are exclusive in some 
> +                # manner
> +                self.assertFalse(wt.is_locked())
> +                self.assertFalse(wt.branch.is_locked())
> +                return

Consider KeyboardInterrupt, and the rule against bare excepts.  I think
in every case we should get a LockError; if any of the classes allow a
bare IOError up we should fix them.

-- 
Martin