[MERGE] Native dirstate update-basis-via-delta

John Arbash Meinel john at arbash-meinel.com
Wed Oct 24 02:16:58 BST 2007


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


...

>> +                self._update_basis_apply_deletes(deletes)
>> +                deletes = []
>>
>> ^- In the inner loop you seem to apply all deletes whenever you see a renamed
>> file. This seems a little odd to me, though I won't say it is incorrect.
> 
> The comment right above it explains why in it's first sentence; if you
> could see why it didn't gel for you that would be great.

Because you wrote a small book. And the language is a bit convoluted. Maybe just:

# Expunge deletes that we've seen so that deleted children of a renamed
# directory are properly handled.

>> Explicitly using:
>>
>> absent = ('r', 'a')
>> ...
>> if kind not in absent:
>>   ...
>>
>> Is probably slower than either
>>

...

> 
> Any specific area? We have a number of tests of this already -
> wt_implementations.get_parents tests just about every permutation I
> could think of.

Ok, there does seem to be sufficient testing. Just that you were adding a patch
but not really adding a lot of tests.
It seems like the tests were actually added for the inventory-based form a
while ago, and now there is a custom WT4 implementation.

The one I didn't see tested was swapping a parent with a child. eg:

 bzr mv foo/bar bar
 bzr mv foo bar/foo

You did
 bzr mv foo tmp
 bzr mv bar foo
 bzr mv tmp bar

Which is close, but not quite the same. I believe the parent to child swap is
the one that forces a lot of the deletes before adds work.

Otherwise, I think you covered this, just not directly in this patch.

> 
>> One problem we have with dirstate, is that if the basis inventory gets skewed
>> from reality, it can do nasty things, and we have very limited ability to
>> detect it.
>>
>> Probably 'bzr check' should assert that "tree.basis_tree()" gives the same
>> result at "tree.branch.repository.revision_tree(tree.last_revision())". That
>> would at least go a way towards giving security in something like this which is
>> mutating over time. (There is no point where we forcibly "sync" and verify that
>> everything is 100% correct.)
> 
> I think thats a good idea. Also bzr check should call tree._validate().
> 
> Do you want these done as part of this patch?

I would like the comparison to be done. It at least gives us a tool in case we
suspect something is broken.

> 
> Also, do you think the _id_index should be adjusted/reset by this? I
> think resetting it is ok and sufficient.
> 
> -Rob

It is necessary and sufficient to reset it. Adjusting it is far too much work
(because of all the edge cases).

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFHHp0JJdeBCYSNAAMRAvQ5AKCogWt3BqHnusxALxVaOEajaWDkAQCbBO/T
MZipIiwxMNywmixrIaqz3eg=
=qKt4
-----END PGP SIGNATURE-----



More information about the bazaar mailing list