[MERGE] Makeing WorkingTree nicer to derive from.

[John Arbash Meinel]

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

Robert Collins wrote:
> On Sat, 2006-07-15 at 10:08 -0500, John Arbash Meinel wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>

...

>>
>> Although... We probably could make the stat field fixed size. And the
>> sha field already is fixed. Since the revision parents don't change
>> (unless you do a merge), we probably could actually replace things
>> inline, rather that rewriting the whole file....
> 
> I thought the plan was to compare the hash cache data as binary blobs,
> so they will be fixed size after either uuencoding or hexlifying, and
> thus updatable in place.

Well, the first part is true. I'm not positive if they are fixed size at
this point. :) I struct.pack() and then base64.encode, so they should
be. I just wanted to be sure that they were truly fixed size, rather
than 'almost' fixed.

Ahh.. I know. The file size is unpacked because it is something we might
care about comparing directly. (versus mtime, inode, etc which we only
care that they haven't changed.). It is actually inside the packed stat,
as well as being outside it.

I left it unpacked because it is an attribute of InventoryFile, so
conceivably we would want it directly. (Though to make parsing fast, I
leave it as a str at this point).

> 
>>> ..
>>>> Rather than putting a try/except around self._hashcache.write() twice,
>>>> it might be better to have a helper function in the base working tree.
>>> or perhaps write() should fail softly.
>> I wasn't sure where the logic that a failure is okay should be. I kind
>> of prefer having WorkingTree know about the failure, but just ignore it.
>>
>> I suppose we could have hashcache.write() return success/failure.
> 
> That would work for me.
> 

So should a failed write be a warning, or just a mutter?

> 
> 
>> You didn't really give an explanation why you changed lock.py. Is it
>> just so that you can prevent multiple locks of the same object? We've
>> allowed that for quite some time.
>> And there were a few tests that depended on it. (Things that would do
>> 'bzr merge -r revid:foo .')
>> I would kind of like to support it. Because it can be handy when 'foo'
>> is not in your ancestry. (I've used it after uncommit/bundles/etc)
> 
> There's nothing in the test suite that is broken by my change to
> lock.py. The change to lock.py makes it do the following:
>  * shared readlocks of foo.
>  * writelocks of foo are exclusive.
> 
> Which is the same as we get with the lockdir formats. 'bzr merge -r
> revid:foo .' still works just fine.
> 
> Supporting multiple writelocks of the same path with different python
> objects is incredibly dangerous - lock instance 1, lock instance 2, do a
> pull in 1, do a commit in 2 and watch the repository get corrupted.
> 

Yeah, I realized this after I had some time to think about it. I'm very
+1 about the changes to locking.

...

> Firstly, both our tests only test one code base. They dont test code
> base A locking a tree and branch, and code base B also locking a tree
> and branch. 
> 
> So ALL the code our tests encounter locks in the same order - TB or BT,
> for any given tree format and lock mode. We can have different orders if
> one instance is locking for read and one for write though.

True. But it means that if someone changes the order of locking, they
are aware that we were expecting the previous order. I understand we
don't test 2 codebases. But we can let people know when they change
something that we expect to work in a certain way.

...

> But all of that considered, I'd much rather make our os locks timeout
> rather than stopping cold, and if we do that deadlocking stops being an
> issue because the thread will return a failure rather than deadlocking.

Well, Win32 locks actually raise an exception rather than blocking.

> 
> Is it possible to have two processes retrying forever with timeouts?
> yes, but I think that is an issue for the caller - if you get a
> LockError, and want to retry, its up to you to randomise a little.
> 
>> I'm fine moving it somewhere else. I do think the test has value on its
>> own, though.
> 
> At this point, I dont - I think its testing something that can change
> without harming anything in the program, and as such theres no need to
> observe it with a test.

I disagree with the second part. It would break consistency with a given
version of the program. (Especially a released version).
I think changing the order of locking should at least be something that
causes a test to fail, which a person can then fix. But shouldn't be
something that happens quietly.


If you are 100% set against the test, I'll survive without it. But I do
still think it is useful.

John
=:->


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

iD8DBQFEu9gRJdeBCYSNAAMRAgdJAJ9rsq0+gRcbe6DpGM1EM8f6TBYJMACdHgmx
iJQn96MhWEQHVHwa2zZHbP4=
=QIjg
-----END PGP SIGNATURE-----