[MERGE] Review feedback on dirstate update_basis_via_delta logic.

John Arbash Meinel john at arbash-meinel.com
Wed Oct 24 18:36:53 BST 2007


John Arbash Meinel has voted tweak.
Status is now: Conditionally approved
Comment:
I wish you had included a delta of the change since your previous 
submission. Since it would make reviewing your incremental changes a lot 
easier.

It seems like we should have two simple runs of 'bzr check', one without 
a working tree, and one with a working tree. Just to catch any typos in 
the with WT version. (I might also argue that we need one with the first 
commit [no basis], and one with at least one commit.)

I feel a little weird using tree._iter_changes() as we have been trying 
hard to get that special cased/optimized. I realize right now it is 
fine, but potentially it could check tree.get_revision_id() and realize 
it can use the fast-path code. While we really just want a very simple 
(and thus very unlikely to be incorrect) code path checking.


The update did reveal something I missed before, which is that the 
documentation is not quite complete. You added a field, but that didn't 
show up everywhere, and here:
          :param adds: A sequence of deletes. Each delete is a tuple:
-            (old_path_utf8, None, file_id, None)
+            (old_path_utf8, new_path_utf8, file_id, None, real_delete).
+            real_delete is True when the desired outcome is an actual 
deletion
+            rather than the rename handling logic temporarily deleting 
a path
+            during the replacement of a parent.

I'm pretty sure that ":param adds:" should not be talking about deletes. 
So I think you need to update the docstring on _update_basis_apply_adds, 
and _update_basis_apply_deletes.
Otherwise the changes look good.

For details, see: 
http://bundlebuggy.aaronbentley.com/request/%3C1193205019.3413.2.camel%40lifeless-64%3E



More information about the bazaar mailing list