Merge lp://staging/~jameinel/bzr/2.4-cheaper-iter-entries-by-dir into lp://staging/bzr

That was meant to be 70k entries.

Somebody recently mentioned to me that we don't allow the use of "assert"... :-)

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

On 3/18/2011 5:36 PM, Jelmer Vernooij wrote:
> Somebody recently mentioned to me that we don't allow the use of "assert"... :-)

You're absolutely right, I meant to pull those out after I had it working.

I updated it to raise ValueError on any of the inconsistencies. None of
them should ever trigger, and I wonder if we really want to check them.
But since I did it, I'm ok leaving it in. It probably makes the code a
little bit harder to follow, though.

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

iEYEARECAAYFAk2EZ50ACgkQJdeBCYSNAAOSuQCfQw0i9e8P/t+z05Cnj7GpxO2I
MywAnjaoZWUX50HQvui+OyYfGXoOA9R1
=nAEZ
-----END PGP SIGNATURE-----

29 + if from_dir is None and specific_file_ids is None:
30 + # They are iterating from the root, and have not specified any
31 + # specific entries to look at. All current callers fully consume the
32 + # iterator, so we can safely assume we are accessing all entries
33 + self._preload_cache()
34 if from_dir is None:
35 if self.root is None:
36 return

You're pre-loading the cache even when from_dir and self.root are None, is there nothing to preload in this case or does it mean the shortcut is now ineffective and what are the consequences then ?

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

On 3/20/2011 12:18 PM, Vincent Ladeuil wrote:
> Review: Needs Information
> 29 + if from_dir is None and specific_file_ids is None:
> 30 + # They are iterating from the root, and have not specified any
> 31 + # specific entries to look at. All current callers fully consume the
> 32 + # iterator, so we can safely assume we are accessing all entries
> 33 + self._preload_cache()
> 34 if from_dir is None:
> 35 if self.root is None:
> 36 return
>
> You're pre-loading the cache even when from_dir and self.root are None, is there nothing to preload in this case or does it mean the shortcut is now ineffective and what are the consequences then ?
>

If self.root is None, there shouldn't be anything to preload, because if
you have no root, you better not have any children of root.

That said, I'm not 100% sure that CHKInventory can exist without a root.
If you had an object like that, it would probably have to be an
Inventory object.

Now, iter_entries_by_dir() exists only on CommonInventory, so it is
shared between both types. However the base implementation of _preload
is also a no-op.

So if you have a tree with no root, then preload is a no-op (one way or
the other).

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

iEYEARECAAYFAk2GTGAACgkQJdeBCYSNAANO1ACfWVNV/NeV9PqMmB7Gh6Hl0sxU
JckAoNFKKICCU0Zl6ku0VfGYSzPUHzWL
=KmmW
-----END PGP SIGNATURE-----

It's rather weird to have an iterator turned into a full loading method even for a special case...

But since this is a significant progress I think we should land it. A further submission clarifying the different use cases would certainly be welcome.

Is this a reasonable thing to backport? It seems like it would be very beneficial to have this at least in natty.

Are there perhaps other locations where preloading would make sense? E.g. Inventory.iter_entries(from_dir=None) ?

sent to pqm by email

I think we could reasonable back port this to at least 2.3, what do other people think?

sent to pqm by email

John A Meinel wrote:
> I think we could reasonable back port this to at least 2.3, what do other people think?

Sounds reasonable to me, assuming you're confident that there's little
risk involved.

-Andrew.