Merge lp:~bzr/bzr/iter-entries-norecurse into lp:~bzr/bzr/trunk-old

Proposed by Ian Clatworthy on 2009-05-18
Status: Merged
Approved by: John A Meinel on 2009-06-01
Approved revision: 4371
Merged at revision: not available
Proposed branch: lp:~bzr/bzr/iter-entries-norecurse
Merge into: lp:~bzr/bzr/trunk-old
Diff against target: 84 lines
To merge this branch: bzr merge lp:~bzr/bzr/iter-entries-norecurse
Reviewer Review Type Date Requested Status
John A Meinel 2009-05-18 Approve on 2009-06-01
Review via email: mp+6663@code.launchpad.net
To post a comment you must log in.
Ian Clatworthy (ian-clatworthy) wrote :

This patch extends Inventory.iter_entries() with a parameter making recursing down optional. This is used by my patches-under-development that speed up selective file commit and the ls command. As such, I'd appreciate a review soon so I can complete the follow-on patches.

John A Meinel (jameinel) wrote :

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

Ian Clatworthy wrote:
> Ian Clatworthy has proposed merging lp:~bzr/bzr/iter-entries-norecurse into lp:bzr.
>
> Requested reviews:
> Bazaar Developers (bzr)
>
> This patch extends Inventory.iter_entries() with a parameter making recursing down optional. This is used by my patches-under-development that speed up selective file commit and the ls command. As such, I'd appreciate a review soon so I can complete the follow-on patches.
>

I'll note that if you use 'iter_entries_by_dir()' you don't have to
worry about passing 'no_recurse=True', you can just stop iterating once
you find you are no longer in that directory. Just a thought.

  Review:comment

John
=:->

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

iEYEARECAAYFAkoQ358ACgkQJdeBCYSNAANa0QCgvqwr+1PDmVd42KWoNJC0nq88
ickAoKGT0c6FEdx4GZyZ5hXCuDHUEmhh
=xf+w
-----END PGP SIGNATURE-----

Ian Clatworthy (ian-clatworthy) wrote :

> I'll note that if you use 'iter_entries_by_dir()' you don't have to
> worry about passing 'no_recurse=True', you can just stop iterating once
> you find you are no longer in that directory. Just a thought.

That's true but it seems more fragile to me. (I'm not confident that all implementations of iter_entries_by_dir() will strictly implement the ordering in the future.) By making it a parameter, the behaviour is more explicit and easier to test IMO.

John A Meinel (jameinel) wrote :

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

Ian Clatworthy wrote:
>> I'll note that if you use 'iter_entries_by_dir()' you don't have to
>> worry about passing 'no_recurse=True', you can just stop iterating once
>> you find you are no longer in that directory. Just a thought.
>
> That's true but it seems more fragile to me. (I'm not confident that all implementations of iter_entries_by_dir() will strictly implement the ordering in the future.) By making it a parameter, the behaviour is more explicit and easier to test IMO.

iter_entries_by_dir has strictly enforced ordering so that you can walk
iterators side-by-side. The ordering *is* part of the api.

That said, a flag certainly seems a bit more obvious.

John
=:->

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

iEYEARECAAYFAkoRcDIACgkQJdeBCYSNAAO0IQCeIrf3ZdqZxV92gtyU+VoDSjxh
XP4An0TuM9IpJceLtusMBFa/RaJGay6K
=NRKc
-----END PGP SIGNATURE-----

Robert Collins (lifeless) wrote :

On Mon, 2009-05-18 at 03:20 +0000, Ian Clatworthy wrote:
> Ian Clatworthy has proposed merging lp:~bzr/bzr/iter-entries-norecurse into lp:bzr.
>
> Requested reviews:
> Bazaar Developers (bzr)
>
> This patch extends Inventory.iter_entries() with a parameter making
> recursing down optional. This is used by my patches-under-development
> that speed up selective file commit and the ls command. As such, I'd
> appreciate a review soon so I can complete the follow-on patches.

I haven't read the code [yet]. A few comments:
 - with respect to commit and so on, I was actually suggesting doing
this under the hood: we _really_ don't want to exit out of iter_changes
and back into it. Doing so will lose state calculated about what we've
seen, and may lead to race conditions updating the hash cache.
 - This is a fine feature to have in its own right

-Rob

John A Meinel (jameinel) wrote :

I would probably say that we should use "iter_entries_by_dir" whenever possible, as that is the optimal order for walking directories, etc.

I don't have a specific problem with this change, as it does make the request explicit.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/inventory.py'
2--- bzrlib/inventory.py 2009-05-26 10:08:17 +0000
3+++ bzrlib/inventory.py 2009-06-02 19:37:38 +0000
4@@ -747,8 +747,13 @@
5 [parent.name for parent in
6 self._iter_file_id_parents(file_id)][:-1]))
7
8- def iter_entries(self, from_dir=None):
9- """Return (path, entry) pairs, in order by name."""
10+ def iter_entries(self, from_dir=None, recursive=True):
11+ """Return (path, entry) pairs, in order by name.
12+
13+ :param from_dir: if None, start from the root,
14+ otherwise start from this directory (either file-id or entry)
15+ :param recursive: recurse into directories or not
16+ """
17 if from_dir is None:
18 if self.root is None:
19 return
20@@ -761,6 +766,10 @@
21 # 440ms/663ms (inline/total) to 116ms/116ms
22 children = from_dir.children.items()
23 children.sort()
24+ if not recursive:
25+ for name, ie in children:
26+ yield name, ie
27+ return
28 children = collections.deque(children)
29 stack = [(u'', children)]
30 while stack:
31
32=== modified file 'bzrlib/tests/inventory_implementations/basics.py'
33--- bzrlib/tests/inventory_implementations/basics.py 2009-04-09 20:23:07 +0000
34+++ bzrlib/tests/inventory_implementations/basics.py 2009-06-02 19:37:38 +0000
35@@ -211,8 +211,12 @@
36 ('doc', 'directory', 'doc-id'),
37 ('src/hello.c', 'file', 'hello-id'),
38 ('src/bye.c', 'file', 'bye-id'),
39+ ('src/sub', 'directory', 'sub-id'),
40+ ('src/sub/a', 'file', 'a-id'),
41 ('Makefile', 'file', 'makefile-id')]:
42 inv.add_path(*args)
43+
44+ # Test all entries
45 self.assertEqual([
46 ('', 'tree-root'),
47 ('Makefile', 'makefile-id'),
48@@ -220,8 +224,36 @@
49 ('src', 'src-id'),
50 ('src/bye.c', 'bye-id'),
51 ('src/hello.c', 'hello-id'),
52+ ('src/sub', 'sub-id'),
53+ ('src/sub/a', 'a-id'),
54 ], [(path, ie.file_id) for path, ie in inv.iter_entries()])
55
56+ # Test a subdirectory
57+ self.assertEqual([
58+ ('bye.c', 'bye-id'),
59+ ('hello.c', 'hello-id'),
60+ ('sub', 'sub-id'),
61+ ('sub/a', 'a-id'),
62+ ], [(path, ie.file_id) for path, ie in inv.iter_entries(
63+ from_dir='src-id')])
64+
65+ # Test not recursing at the root level
66+ self.assertEqual([
67+ ('', 'tree-root'),
68+ ('Makefile', 'makefile-id'),
69+ ('doc', 'doc-id'),
70+ ('src', 'src-id'),
71+ ], [(path, ie.file_id) for path, ie in inv.iter_entries(
72+ recursive=False)])
73+
74+ # Test not recursing at a subdirectory level
75+ self.assertEqual([
76+ ('bye.c', 'bye-id'),
77+ ('hello.c', 'hello-id'),
78+ ('sub', 'sub-id'),
79+ ], [(path, ie.file_id) for path, ie in inv.iter_entries(
80+ from_dir='src-id', recursive=False)])
81+
82 def test_iter_just_entries(self):
83 inv = self.make_inventory('tree-root')
84 for args in [('src', 'directory', 'src-id'),