[MERGE] Makeing WorkingTree nicer to derive from.
John Arbash Meinel
john at arbash-meinel.com
Sat Jul 15 16:08:24 BST 2006
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Robert Collins wrote:
...
>
>>> class WorkingTree3(WorkingTree):
>>> """This is the Format 3 working tree.
>>>
>>> @@ -1542,6 +1541,15 @@
>>> raise ConflictFormatError()
>>> return ConflictList.from_stanzas(RioReader(confile))
>>>
>>> + def unlock(self):
>>> + if self._hashcache.needs_write and self._control_files._lock_count==1:
>>> + self._hashcache.write()
>>> + # reverse order of locking.
>>> + try:
>>> + return self._control_files.unlock()
>>> + finally:
>>> + self.branch.unlock()
>>> +
>> I like this refactoring, as it does make things cleaner. The only part
>> here, is that 'self._hashcache.write()' needs a try/except around it, in
>> the case that we have a readonly working tree.
>
> I'm happy to add a TODO to that, but its certainly not in the pipeline
> for what I'm working on - which is dirstate. dirstate will remove the
> separate hashcache anyway, so there is no motivation to fix that as part
> of this branch.
Sure. I'm wondering if we want to completely merge the hashcache into
the dirstate. It seems like we will want to avoid rewriting it as often
as we might update the hashcache.
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....
> ..
>> 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.
>
>> Your refactoring is still +1 to come in. But I wouldn't mind discussing
>> whether not being able to write the hashcache is a warning/note/or just
>> a mutter.
>>
>> The specific use case is running 'bzr status' or 'bzr info' on a
>> readonly tree. (bzr diff doesn't seem to update the hashcache in my
>> testing).
>
>
> I looked at the existing lock unlock tests, and have a few key comments
> about them. Firstly, here is a replacement patch for them, to make them
> only test the interface, rather than breaking on other branch types -
> such as 'hg' or 'git' branches.
>
> There is one thing that was being tested that my replacement does not
> test. This is the precise lock and unlock order of the lock() and
> unlock() calls. Now, why am I not testing that anymore? I dont think
> that the order a branch and tree pair decide to cooperate on the lock
> and unlock sequence is an appropriate part of the interface. Its
> possibly something to test in a implementation specific manner - which
> your somewhat complex decorator approach did, but definately not
> something to test that every implementation does. Interestingly, my
> replacement tests exposed a bug - something implicitly tested but not
> explicitly tested, which is that opening a format 6 branch twice in a
> single python process would allow data destruction due to allowing both
> instances to hold a write lock at once. This patch corrects that. This
> is something that is tested as a result of using a second copy of the
> branch to force lock failures.
>
> Anyhow, would like your feedback on this.
>
> Cheers,
> Rob
>
>
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)
...
I also would not like to lose testing format 2 and 3 explicit locking
sequence. I do think it is important to lock in the correct way. I agree
that we don't need every implementation to lock in this exact way. Your
tests are at a level that they can't detect locking order. Which means
one implementation might change order, and cause deadlocks with
something else.
I'm fine moving it somewhere else. I do think the test has value on its
own, though.
I think your tests are a good addition. And probably better as a generic
interface test. But they are testing something different than what I was
testing.
We don't have a good way with our adapters to say 'test *these*
implementations, but not *these* implementations'. And I realize with an
interface test, you really aren't supposed to do that.
But we do have a cause for testing certain implementations, and DRY
would indicate we shouldn't copy the tests. And you don't seem to like
mixins...
Any suggestions?
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2.2 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFEuQToJdeBCYSNAAMRAhz5AKCoioIzKKssoEdYQ5EFpqbZsUuoRACfSSKg
v1on9MCOKDN0/aihSAYv8TE=
=Me1U
-----END PGP SIGNATURE-----
More information about the bazaar
mailing list