[MERGE] Add an optional 'token' keyword argument to LockableFiles.lock_write

[Andrew Bennetts]

Robert Collins wrote:
> On Thu, 2007-04-12 at 15:37 +1000, Andrew Bennetts wrote:
> 
> > I'm also a little bit uncertain about the "leave_lock_in_place" and
> > "dont_leave_lock_in_place" APIs; it would be possible to slice this problem
> > differently to allow more direct manipulation of whether or not an object thinks
> > its locked, but that perhaps would make it easier to get objects into invalid
> > states.
> 
> I think it would. The current api is very hard to force into invalid
> behaviour, and efficient. A more flexible API would probably end up
> doing multiple disk reads in the common case which we should avoid.

Yeah.  The current API is good enough, even if it isn't perfect, so I'll just
try to put it out of my mind until someone comes up with a concrete suggestion
to improve it.

> +    * The ``lock_write`` method of ``LockableFiles`` and ``Repository`` now
> +      accept a ``token`` keyword argument, so that a seperate instances of those
> +      objects can share a lock if it has the right token.  (Andrew Bennetts,
> +      Robert Collins)
> 
> 'separate'.

Fixed.

> +    * ``Branch.lock_write()`` now accepts a ``tokens`` keyword argument, which
> +      is like the other ``lock_write`` methods, except that it also allows for
> +      sharing a repository lock.  (Andrew Bennetts, Robert Collins)
> 
> I think that Branch.lock_write should only accept a single token - so the expected use will be:
> branch.repository.lock_write(repo_token)
> branch.lock_write(branch_token)
> 
> This makes the branch lock_write simpler and more likely to be factorable out in the future.

I've done this.  It makes a couple of things in the hpss branch a little uglier,
and makes some tests slightly uglier too, but in the end it was probably worth
it.  I'm confident the resulting warts can be fixed if they turn out to be real
problems (e.g. add "lock_write_with_repo_token" and "get_repo_token" methods to
Branch).


> +            assert tokens == (None, None) or None not in tokens, (
> +                'Both branch and repository locks must return tokens, or else '
> +                'neither must return tokens.  Got %r.' % (tokens,))
> 
> I dont this assertion is useful. Making Branch.lock_write only return its own
> token would make it irrelevant anyway.

It is now irrelevant, and hence gone.

> +    def dont_leave_in_place(self):
> +        """Set this LockableFiles to clear the physical lock on unlock."""
> +        # XXX: think about renaming this!
> +        self._lock.dont_leave_in_place()
> 
> I dont the the XXX is needed here. If you cant think of a better name,
> just deal - if someone comes up with a better name later, we can adjust
> then.

XXX deleted.

> +            # XXX: add test for the case that requires self._token_from_lock:
> +            # token = x.lock_write(); assert(x.lock_write() == token)
> +            self._token_from_lock = token_from_lock
> +            return token_from_lock
> 
> Is that XXX done? It seems straight forward.

Fixed.

> +    def setUp(self):
> +        TestCaseWithRepository.setUp(self)
> +        self.reduceLockdirTimeout()
> 
> ^^ This is unneeded now.

Fixed.

>  class _TestLockableFiles_mixin(object):
>  
> +    def setUp(self):
> +        self.reduceLockdirTimeout()
> +
> 
> ^^ This is unneeded now.

Fixed.

Thanks!

-Andrew.