branch locking mk2.

[Martin Pool]

John A Meinel wrote:

> I have a couple inline comments. But one overall comment is why call it
> LockFile? Since it is using directories to do the locking it seems misnamed.

Yes, when you put it like that...  Should it just be LockDir or can 
anyone think of a better name?

> The comments here are incorrect. I'm guessing you used to have peek()
> return True/False, but now it returns a dictionary.

Correct.

> test_32_lock_wait_succeed is identical to test_33_wait
> 
> I'm guessing you want to test a LockFile.wait() which fails.

The difference is that 33 calls LockFile.wait(), which waits for it to 
be released but does not acquire it.  This is meant to be used by readers.

> I thought we were avoiding hasattr, prefering to do:
> fn = getattr(self.transport, 'has_atomic_rename', None)
> if not (fn and fn()):
>   rais Unlockable....
> 
> Though are we worried about a platform that doesn't have atomic rename?

I think we're OK everywhere except as you say for disabling fancy_rename.

> I'm not sure what you are doing with wait(). Because 'self.peek()'
> returns information if the lock is held. Which means you return right
> away if the lock is held. Maybe you want 'if not self.peek(): return'.

Yes, the sense is inverted, and the test is deficient that it didn't fail.

> Also, you have no api or tests for breaking a lock that is held by
> someone else.

Yes.

> But the basics look pretty good here.

Thanks for reviewing it.

-- 
Martin