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

[Ian Clatworthy]

[Ian Clatworthy (ian-clatworthy)]

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)]

-----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)]

> 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)]

-----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)]

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)]

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

=== modified file 'bzrlib/inventory.py'
--- bzrlib/inventory.py	2009-05-26 10:08:17 +0000
+++ bzrlib/inventory.py	2009-06-02 19:37:38 +0000
@@ -747,8 +747,13 @@
             [parent.name for parent in
              self._iter_file_id_parents(file_id)][:-1]))
 
-    def iter_entries(self, from_dir=None):
-        """Return (path, entry) pairs, in order by name."""
+    def iter_entries(self, from_dir=None, recursive=True):
+        """Return (path, entry) pairs, in order by name.
+        
+        :param from_dir: if None, start from the root,
+          otherwise start from this directory (either file-id or entry)
+        :param recursive: recurse into directories or not
+        """
         if from_dir is None:
             if self.root is None:
                 return
@@ -761,6 +766,10 @@
         # 440ms/663ms (inline/total) to 116ms/116ms
         children = from_dir.children.items()
         children.sort()
+        if not recursive:
+            for name, ie in children:
+                yield name, ie
+            return
         children = collections.deque(children)
         stack = [(u'', children)]
         while stack:
 
=== modified file 'bzrlib/tests/inventory_implementations/basics.py'
--- bzrlib/tests/inventory_implementations/basics.py	2009-04-09 20:23:07 +0000
+++ bzrlib/tests/inventory_implementations/basics.py	2009-06-02 19:37:38 +0000
@@ -211,8 +211,12 @@
                      ('doc', 'directory', 'doc-id'),
                      ('src/hello.c', 'file', 'hello-id'),
                      ('src/bye.c', 'file', 'bye-id'),
+                     ('src/sub', 'directory', 'sub-id'),
+                     ('src/sub/a', 'file', 'a-id'),
                      ('Makefile', 'file', 'makefile-id')]:
             inv.add_path(*args)
+
+        # Test all entries
         self.assertEqual([
             ('', 'tree-root'),
             ('Makefile', 'makefile-id'),
@@ -220,8 +224,36 @@
             ('src', 'src-id'),
             ('src/bye.c', 'bye-id'),
             ('src/hello.c', 'hello-id'),
+            ('src/sub', 'sub-id'),
+            ('src/sub/a', 'a-id'),
             ], [(path, ie.file_id) for path, ie in inv.iter_entries()])
 
+        # Test a subdirectory
+        self.assertEqual([
+            ('bye.c', 'bye-id'),
+            ('hello.c', 'hello-id'),
+            ('sub', 'sub-id'),
+            ('sub/a', 'a-id'),
+            ], [(path, ie.file_id) for path, ie in inv.iter_entries(
+            from_dir='src-id')])
+
+        # Test not recursing at the root level
+        self.assertEqual([
+            ('', 'tree-root'),
+            ('Makefile', 'makefile-id'),
+            ('doc', 'doc-id'),
+            ('src', 'src-id'),
+            ], [(path, ie.file_id) for path, ie in inv.iter_entries(
+            recursive=False)])
+
+        # Test not recursing at a subdirectory level
+        self.assertEqual([
+            ('bye.c', 'bye-id'),
+            ('hello.c', 'hello-id'),
+            ('sub', 'sub-id'),
+            ], [(path, ie.file_id) for path, ie in inv.iter_entries(
+            from_dir='src-id', recursive=False)])
+
     def test_iter_just_entries(self):
         inv = self.make_inventory('tree-root')
         for args in [('src', 'directory', 'src-id'),