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

[Andrew Bennetts]

Martin Pool wrote:
> On 12 Feb 2007, Andrew Bennetts <andrew at canonical.com> wrote:
> > +class TokenMismatch(LockError):
> > +
> > +    _fmt = "The lock token %(given_token)r does not match lock token %(lock_token)r."
> > +
> > +    internal_error = True
> > +
> > +    def __init__(self, given_token, lock_token):
> > +        self.given_token = given_token
> > +        self.lock_token = lock_token
> > +
> > +
> >  class PointlessCommit(BzrError):
> 
> I think you can be more clear here and in the other docstrings about
> what has happened in this case: does it mean that someone else broke the
> lock between when you got the initial token and when you tried to
> continue the lock?

A token should never be passed unless the caller has already acquired a lock
somehow.  So this would usually mean that there's been a programming error, and
the wrong token is being passed in.  You are right that it could also happen if
the lock has been broken by someone else, and I've updated the class to inherit
from LockBroken to reflect this.

> > === modified file bzrlib/lockable_files.py
> > --- bzrlib/lockable_files.py
> > +++ bzrlib/lockable_files.py
> > @@ -212,21 +212,33 @@
> >              raise errors.BzrBadParameterNotString(a_string)
> >          self.put(path, StringIO(a_string.encode('utf-8')))
> >  
> > -    def lock_write(self):
> > +    def lock_write(self, token=None):
> > +        """Lock this group of files for writing.
> > +        
> > +        :param token: if this is already locked, then lock_write will fail
> > +            unless the token matches the existing lock.
> > +        :returns: a token if this instance supports tokens, otherwise None.
> > +        :raises TokenLockingNotSupported: when a token is given but this
> > +            instance doesn't support using token locks.
> > +        :raises MismatchedToken: if the specified token doesn't match the token
> > +            of the existing lock.
> > +        """
> 
> Could do with a couple of sentences about why you might want to pass the
> token and where it comes from.
> 
> I note in passing that having to pass this through seems like additional
> reason to remove LockableFiles and just talk to either the transport or
> the lock directly.

I've added this paragraph to the docstring:

        A token should be passed in if you know that you have locked the object
        some other way, and need to synchronise this object's state with that
        fact.  For instance, this could happen when accessing the same branch
        over bzr+ssh:// and then falling back to do some operations on the same
        branch via sftp://.
         
> > @@ -457,3 +479,14 @@
> >              'locked %s' % (format_delta(delta),),
> >              ]
> >  
> > +    def validate_token(self, token):
> > +        if token is not None:
> > +            info = self.peek()
> > +            if info is None:
> > +                # Lock isn't held
> > +                lock_token = None
> > +            else:
> > +                lock_token = info.get('nonce')
> > +            if token != lock_token:
> > +                raise errors.TokenMismatch(token, lock_token)
> > +
> 
> Why not use the whole info object as the token?  Doing so makes it a
> little more sure we won't collide and might help debug any problems.

As said in an earlier mail, this is a good idea, but for the sake of landing
this now I'll leave it as is for now.  We can fairly easily change this later;
the token is just an arbitrary string, and if they start being returned as a
longer string encoding some extra debug info rather than just the nonce nothing
will break.

Deferring this also means I can defer worrying about encoding issues for binary
data (like \x01) in the hpss protocol for a little bit longer...

-Andrew.