[MERGE] Review feedback on dirstate update_basis_via_delta logic.

Robert Collins robertc at robertcollins.net
Wed Oct 24 21:11:27 BST 2007


On Wed, 2007-10-24 at 13:36 -0400, John Arbash Meinel wrote:
> 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.

Well, diff -r -2..-1 ;). It is a bundle after all.

> 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'll add one with a wt and with a 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.

This is a possible concern. Looking at it, I think we can change the
order of tree_basis and repo_basis so the repository revision tree
object is the one asked to diff. And the fast path we care about is
tree->basis, not basis->arbitrary version.

> 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.

Thanks.

Martin, what do you think of this patch? Its a big performance boost,
but given that it's in fairly core code and potentially still has
unknown bugs (I'm not aware of any), it is late in the 0.92 cycle for
it.

We could merge it now, or we could merge it without the change to
commit.py to enable the fast path, or we could just wait for 0.93 to
open.

-Rob
-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20071025/db73bfd3/attachment.pgp 


More information about the bazaar mailing list