[BUG] Pull command on Windows: we have 2 problems

[John A Meinel]

Aaron Bentley wrote:
> John A Meinel wrote:
>>> I think the basic problem is that there are 2 branches that are being
>>> grabbed. The merge code is creating its own Branch object, rather than
>>> re-using the one used by "pull".
> 
> Okay, so these are the sort of locking composition problems that
> originally led me to suggest a system of lock singletons back when we
> were first discussing it.
> 
>>> Here are possible fixes:
>>>
>>> 1) Change the merge interface to take a Branch object, instead of just a
>>> path
>>> 2) Make sure the pull code has destroyed its Branch object before
>>> calling merge (this is difficult, because I believe it wants to use it
>>> afterwards, though it could always just grab it again).
> 
> I see those as workarounds.  I'd add
> 3) change Branch so that read-locked operations will work when Branch is
> write-locked.

Well, if a branch knows that it is write locked, it doesn't have a
problem. Calling "lock_read()" on a branch that is write locked is not
an error. (calling lock_write() or a branch that is read-locked is,
since it would have to upgrade the lock, which we disallow).

The trick is to have the branch know whether it is or is not locked.

Back in the day, I proposed making Branch objects singletons, which was
rejected, because some people might not want to use them as such.

I think switching that to making Locks singletons is fine, as long as
the Branch.lock_* interface is capable of figuring out what the current
lock state of a branch is. (If Branch objects b1 and b2 point to the
same tree, and b1 locks a branch, b2 should be aware that it is now
actually locked).

This probably just requires changing locks so that they can be queried.

> 
>>> I think the basic difference is that on Linux, if you already have a
>>> write lock on a file, it silently ignores the request for a read lock.
> 
> We can emulate that by factoring out the lock into a LocationLock
> object, and keeping a global dict of LocationLocks.  I think factoring
> out LocationLock would be a positive step, no matter what.
> 

I'll agree to that.

>>> Probably Aaron should do the changes so that the merge is done using an
>>> actual Branch object, rather than a path.
>>> I know "merge.merge()" is designed for easy command line interpretation,
>>> and there is a different function for internal bzr code. 
> 
> Actually, pull's already using that internal function.  The problem is
> in there.
> 
> But the attached patch will probably fix it.

This patch should probably be applied, as well as creating
LocationLocks. I'm guessing a weakref dictionary, mapping absolute paths
to location to their lock objects would be sufficient.

John
=:->

> 
> Aaron



-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 249 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20051026/4c14002a/attachment.pgp