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

[John Arbash Meinel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Andrew Bennetts wrote:

...

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


hmm.. falling back to sftp:// after bzr+ssh:// seems a bit weird. (As in
you aren't ever going to do that). But I could be wrong.

One other use-case, though. Is that you might have a repository open,
and you are opening a new one. And you don't *know* whether it is the
same repository or not. (You have a heavy checkout and are opening the
master branch, which may or may not be on the same filesystem, or might
be a loopback over sftp/bzr+ssh onto the same filesystem).

So you *might* be passing a token as "I might already have this open,
but I might not". Is there a reasonable way to do that?

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

Well, if this is going to be changing, it sounds like it needs to change
before an official release. Since you might be using a data structure
rather than a "arbitrary string".

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.3 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFGCtpAJdeBCYSNAAMRAhNrAJ93ehElTXH251p37RS577PT42khXwCdHk1x
XJYFH51NdIMmvqw6cdTvmHc=
=zUGu
-----END PGP SIGNATURE-----