[MERGE] Inventory-free checkout
Aaron Bentley
aaron at aaronbentley.com
Thu Jan 3 21:28:12 GMT 2008
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
John Arbash Meinel wrote:
> John Arbash Meinel has voted tweak.
> Status is now: Conditionally approved
> Comment:
> + def _apply_removals(self, removals):
> + for path in sorted(removals, reverse=True):
> + dirname, basename = osutils.split(path)
> + block_i, entry_i, d_present, f_present = \
> + self._get_block_entry_index(dirname, basename, 0)
> + entry = self._dirblocks[block_i][1][entry_i]
> + entry[1][0] = ('a', '', 0, False, '')
>
> ^- I think apply_removals should be using "DirState._make_absent". As it
> handles when the removal is "permanent". (For example, renaming a file 2
> times, deleting a file which was just added, etc.)
Thanks for the pointer. The DirState code is still very unfamiliar.
> - active_tree_ids = set((f for f in self._tree.inventory if
> + active_tree_ids = set((f for f in
> self._tree.iter_all_file_ids() if
> f not in removed_tree_ids))
>
> ^- This seems better written as:
> active_tree_ids = set(self._tree.iter_all_file_ids())
> active_tree_ids.difference_update(removed_tree_ids)
>
> or even
> active_tree_ids = set(self._tree.iter_all_file_ids()) - removed_tree_ids
Okay, will update.
> I do wonder if "iter_all_file_ids()" is the best interface, given our
> recent concerns about 'iter' performance.
Oh, I wasn't aware there were big concerns. I would be happy with an
API that returned a set or list, too. But we do need a way to get the
exhaustive list.
> - if len(wt.inventory) > 1: # more than just a root
> + if len(list(wt)) > 1: # more than just a root
>
> ^- It seems like there should be a better way to find out that we have
>>=2 entries than building up a list of all 55k entries.
There are more efficient ways, but if there are >=2 entries, we'll error
out, so I thought efficiency wasn't very important, and I'd go for clarity.
> The biggest thing is using _make_absent to make sure all references are
> properly cleaned up, rather than just setting the field to
> NULL_PARENT_DETAILS
Will do. Do you also want me to change _iter_all_file_ids to something
else? Set? List? And if you want me to change the len(list(wt)) > 2,
I'll do that too.
Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFHfVNs0F+nu1YWqI0RAk81AJ0eZ3qlVvYJvvUVncEPstfIjU3F/gCeJ5BY
hnBg5Dl7M4MhoUQCGAhGnDE=
=rU8c
-----END PGP SIGNATURE-----
More information about the bazaar
mailing list