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

[John Arbash Meinel]

Andrew Bennetts wrote:
> John Arbash Meinel wrote:
> [...]
>>> 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.
> 
> That's true.  But the smart server branch does something very similiar; it falls
> back to using a branch object backed on to a vfs transport (i.e. the 0.11
> "smart" protocol) if the RemoteBranch doesn't have a smart protocol
> implementation for that operation.  Ditto RemoteRepositories.

I certainly understand :). But I'm just pointing out that it is trying
to use a fairly contrived example. It may be the easiest way to explain
it...

> 
>> 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?
> 
> Like this, I suppose:
> 
>     try:
>         repo.lock_write(token=token)
>     except TokenMismatch:
>         token = repo.lock_write()
> 


So does Branch.lock_write() return a potential token for the underlying
Repository.lock_write() call? From what I could see of the code, I would
need to do:

def commit(self, a_branch):

  master_branch = a_branch.get_master_branch()
  repo_token = a_branch.repository.lock_write()
  branch_token = a_branch.lock_write()
  try:
    try:
      master_branch.repository.lock_write(token=repo_token)
    except errors.TokenMismatch:
      master_branch.repository.lock_write()
      same_repo = False
    else:
      same_repo = True

    master_branch.lock_write()
    try:
      # All of the commit logic which inserts the text into the
      # repository, and makes sure the branch, repo, and master
      # are properly updated goes here
    finally:
      master_branch.unlock()
      master_branch.repository.unlock()
  finally:
    a_branch.unlock()
    a_branch.repository.unlock()


Which means that your proposed api *can* be used to support heavyweight
checkouts which are sharing a repository. But it is kind of .... ugly?

Anyway, you aren't focused on my use case, you care more about HPSS. I
just wanted to mention a use case for lock tokens, so we can think about
them specifically.

...

> The current API is "arbitrary string that uniquely identifies the lock".  The
> future API would be a "string that encodes the lock info (e.g. the raw bytes
> of the lock/info file) that uniquely identifies the lock."  So code written
> with today's token API will still work just fine with the new one, so long as
> the new one preserves the properties that 
>    1) "type(token) is str" is always true (or "token is None" for objects that
>       don't support lock tokens), and
>    2) "token_a == token_b" iff they are referring to the same lock.
> 
> This is why I say this improvement doesn't need to happen before this can land.
> 

Well, it seems like it might be nice to have it be something other than
"arbitrary string". Which is why it should be discussed. (I guess you
can argue that it has to be an arbitrary string so it can be passed to
the smart server?)


> That said, I would definitely like to do this further work in time for 0.16,
> which would mean no released bzr would have just arbitrary lock tokens w/o debug
> info, but at the moment I want to focus on getting the smart server landed
> before working on "nice to have"s like this.
> 
> -Andrew.

Oh, and the other big api compatibility issue is if you have a smart
server which is returning nonce lock tokens, and a local client that is
expecting debug-info lock tokens. But as long as the client is only
passing them back to the server, I guess it doesn't matter.

(It would specifically matter for the case of switching from bzr+ssh://
to sftp://, but as that isn't likely to ever happen...)

John
=:->