[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