Merge lp:~amanica/bzr/find_bzrdirs-ignore-PermissionDenied-dirs into lp:bzr

[Marius Kruger]

find_bzrdirs should ignore dirs that raises PermissionDenied. (Marius Kruger)

bzrtools' bzr branches crashes if it tries to walk subdirs on which the current user does not have permissions.
So find_bzrdirs should ignore dirs that raises PermissionDenied.

other notes:
* I moved TestCaseWithMemoryTransport.make_smart_server TestCaseWithTransport,
  because I needed it in a non-memory-transport-test, and I can't see anything that makes this specific to memory-transport-tests.
  I also made it possible to specify a backing_server.
* I used a PathFilteringServer to raise PermissionDenied for arbitrary paths, because chmod is not available on all platforms (on recommendation from Andrew - thanks!).

[lp:~amanica/bzr/find_bzrdirs-ignore-PermissionDenied-dirs]

5222. By Marius Kruger <email address hidden> on 2010-05-11

qualify find_bzrdirs in NEWS

[Andrew Bennetts (spiv)]

Thanks for working on this!

 review needs-information

Marius Kruger wrote:
[...]
> other notes:
> * I moved TestCaseWithMemoryTransport.make_smart_server TestCaseWithTransport,
> because I needed it in a non-memory-transport-test, and I can't see anything
> that makes this specific to memory-transport-tests.
> I also made it possible to specify a backing_server.

I don't understand this part. TestCaseWithTransport is a subclass of
TestCaseWithMemoryTransport, so by moving it you've made it available to
strictly fewer tests, not more.

I think you should move it back. The change to optionally pass a
backing_server is fine.

> === modified file 'NEWS'
> --- NEWS 2010-05-06 06:51:11 +0000
> +++ NEWS 2010-05-10 11:08:28 +0000
> @@ -75,6 +75,8 @@
> * ``bzr selftest`` should not use ui.note() since it's not unicode safe.
> (Vincent Ladeuil, #563997)
>
> +* BzrDir.find_bzrdirs should ignore dirs that raises PermissionDenied. (Marius Kruger)

Put double-backticks around the method name, i.e. ``BzrDir.find_bzrdirs``,
to format it as code rather than regular text.

> === modified file 'bzrlib/tests/__init__.py'
> --- bzrlib/tests/__init__.py 2010-05-05 14:11:13 +0000
> +++ bzrlib/tests/__init__.py 2010-05-10 11:08:28 +0000
> @@ -2408,12 +2408,6 @@
> made_control = self.make_bzrdir(relpath, format=format)
> return made_control.create_repository(shared=shared)
>
> - def make_smart_server(self, path):

As stated above, I don't think you should move this.

> === modified file 'bzrlib/tests/test_bzrdir.py'
[...]
> + def make_fake_permission_denied_transport(self, transport, paths):
> + # multiplatform chmod(0000)
> + def filter(path):
> + if path in paths:
> + raise errors.PermissionDenied(path)
> + return path
> + path_filter_server = pathfilter.PathFilteringServer(transport, filter)
> + path_filter_server.start_server()

You should do self.addCleanup(path_filter_server.stop_server) here.

It would be good to add a docstring.

> + path_filter_transport = pathfilter.PathFilteringTransport(
> + path_filter_server, '.')
> + return (path_filter_server, path_filter_transport)
> +
> + def _assert_branch_urls(self, expect_url_suffixes, actual_bzrdirs):
> + "See if each of the actual urls ends with the corresponding suffix"

Style-wise, you should just go ahead and call this assert* rather than
_assert*. And docstrings are always triple-double-quoted. We generally
also use mixedCamelCase for our assertion methods for consistency with
pyunit, but I don't particularly mind if you don't.

“Assert URL” isn't really a meaningful phrase in the way that “assert ends
with” is. It can be hard to find a concise-yet-meaningful name for
assertion methods... although I see you never pass in more than one
suffix/bzrdir pair at a time in the current code, so you could simplify the
name (and implementation) and call it “assert_bzrdir_user_url_has_suffix”.

> + self._assert_branch_urls(['baz/'], list(
> + bzrdir.BzrDir.find_bzrdirs(path_filter_transport)))

e.g. this would be:
        bzrdirs = list(bzrdir.BzrDir.find_b...

review: Needs Information

[Robert Collins (lifeless)]

It might be nice to do a matcher rather than the new complex assert.

-Rob

[Robert Collins (lifeless)]

Let me echo Andrew's plea for more clarity - I'm go to say it with 'Needs Fixing' though - once its more clear we can land this.

If you don't have the time to fix it up, let me know and I'll happily fix the stylistic issues and take my best guess at what the test means - but I'd rather not guess at that. (The style stuff is trivial to do.)

review: Needs Fixing

[Marius Kruger (amanica)]

thanks for the reviews, I got most of the things done on the weekend
but ran out of time while looking at the Matcher stuff.
(but can't push it from work now :-( - no outside ssh allowed).
I'm not sure why the test is unclear, it makes a tree like:
foo/bar
baz
(i.e baz is not under foo)
So if foo is inaccessible for whatever reason, it should still return
baz. Thats the problem I'm trying to fix, at the moment it just blows
up in this scenario.

I found some of Robert's blogs about testtools and Matchers etc,
but I didn't find any president for custom Matchers and Mismatches in
bzrlib. I was going to try it out, but it will help if you can point
me in the right direction. Where should I add custom Matchers and
Mismatches ?

[Robert Collins (lifeless)]

bzrlib.tests.matchers ? :)

[lp:~amanica/bzr/find_bzrdirs-ignore-PermissionDenied-dirs]

5223. By Marius Kruger <email address hidden> on 2010-05-23

move make_smart_server back to TestCaseWithMemoryTransport from TestCaseWithTransport as per review

5224. By Marius Kruger <email address hidden> on 2010-05-23

* Tried to improve code docs and NEWS as per review
* _assert_branch_urls => assertBranchUrlsEndsWith
  - try to clarify what it does with a more explicit name and code doc
  - convert actual_bzrdirs to a list in here for the caller

[Robert Collins (lifeless)]

I'm worried your test may be fragile, now that its a little clearer, it seems likely to me that it depends on filesystem order, and some filesystems return order based on hashing.

You know all the full urls you want, so perhaps just using a set and comparing will be more robust and do the same job?

review: Needs Fixing

=== modified file 'NEWS'
--- NEWS	2010-05-21 07:17:13 +0000
+++ NEWS	2010-05-24 20:06:20 +0000
@@ -97,9 +97,15 @@
 * ``bzr selftest`` should not use ui.note() since it's not unicode safe.
   (Vincent Ladeuil, #563997)
 
+<<<<<<< TREE
 * CommitBuilder refuses to create revisions whose trees have no root.
   (Aaron Bentley)
 
+=======
+* ``BzrDir.find_bzrdirs`` should ignore dirs that raises PermissionDenied.
+  (Marius Kruger)
+
+>>>>>>> MERGE-SOURCE
 * Don't mention --no-strict when we just issue the warning about unclean trees.
   (Vincent Ladeuil, #401599)
 

=== modified file 'bzrlib/bzrdir.py'
--- bzrlib/bzrdir.py	2010-05-20 18:23:17 +0000
+++ bzrlib/bzrdir.py	2010-05-24 20:06:20 +0000
@@ -375,14 +375,14 @@
             recurse = True
             try:
                 bzrdir = BzrDir.open_from_transport(current_transport)
-            except errors.NotBranchError:
+            except (errors.NotBranchError, errors.PermissionDenied):
                 pass
             else:
                 recurse, value = evaluate(bzrdir)
                 yield value
             try:
                 subdirs = list_current(current_transport)
-            except errors.NoSuchFile:
+            except (errors.NoSuchFile, errors.PermissionDenied):
                 continue
             if recurse:
                 for subdir in sorted(subdirs, reverse=True):
@@ -1958,7 +1958,6 @@
             format_string = transport.get_bytes(".bzr/branch-format")
         except errors.NoSuchFile:
             raise errors.NotBranchError(path=transport.base)
-
         try:
             return klass._formats[format_string]
         except KeyError:

=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py	2010-05-20 18:23:17 +0000
+++ bzrlib/tests/__init__.py	2010-05-24 20:06:20 +0000
@@ -2408,9 +2408,11 @@
         made_control = self.make_bzrdir(relpath, format=format)
         return made_control.create_repository(shared=shared)
 
-    def make_smart_server(self, path):
+    def make_smart_server(self, path, backing_server=None):
+        if backing_server is None:
+            backing_server = self.get_server()
         smart_server = test_server.SmartTCPServer_for_testing()
-        self.start_server(smart_server, self.get_server())
+        self.start_server(smart_server, backing_server)
         remote_transport = get_transport(smart_server.get_url()).clone(path)
         return remote_transport
 

=== modified file 'bzrlib/tests/test_bzrdir.py'
--- bzrlib/tests/test_bzrdir.py	2010-03-25 14:22:41 +0000
+++ bzrlib/tests/test_bzrdir.py	2010-05-24 20:06:20 +0000
@@ -54,6 +54,7 @@
 from bzrlib.transport import (
     get_transport,
     memory,
+    pathfilter,
     )
 from bzrlib.transport.http._urllib import HttpTransport_urllib
 from bzrlib.transport.nosmart import NoSmartTransportDecorator
@@ -807,6 +808,43 @@
         self.assertEqualBzrdirs([baz, foo, bar],
                                 bzrdir.BzrDir.find_bzrdirs(transport))
 
+    def make_fake_permission_denied_transport(self, transport, paths):
+        """Create a transport that raises PermissionDenied for some paths."""
+        def filter(path):
+            if path in paths:
+                raise errors.PermissionDenied(path)
+            return path
+        path_filter_server = pathfilter.PathFilteringServer(transport, filter)
+        path_filter_server.start_server()
+        self.addCleanup(path_filter_server.stop_server)
+        path_filter_transport = pathfilter.PathFilteringTransport(
+            path_filter_server, '.')
+        return (path_filter_server, path_filter_transport)
+
+    def assertBranchUrlsEndsWith(self, expect_url_suffixes, actual_bzrdirs):
+        """Check that each branch url ends with the corresponding suffix"""
+        actual_bzrdirs = list(actual_bzrdirs)
+        self.assertEqual(len(expect_url_suffixes), len(actual_bzrdirs))
+        for expect_url_suffix, actual_bzrdir in zip(
+                expect_url_suffixes, actual_bzrdirs):
+            self.assertEndsWith(actual_bzrdir.user_url, expect_url_suffix)
+
+    def test_find_bzrdirs_permission_denied(self):
+        foo, bar, baz = self.make_foo_bar_baz()
+        transport = get_transport(self.get_url())
+        (path_filter_server, path_filter_transport
+            ) = self.make_fake_permission_denied_transport(transport, ['foo'])
+
+        # local transport
+        self.assertBranchUrlsEndsWith(['baz/'],
+            bzrdir.BzrDir.find_bzrdirs(path_filter_transport))
+
+        # smart server
+        smart_transport = self.make_smart_server('.',
+            backing_server=path_filter_server)
+        self.assertBranchUrlsEndsWith(['baz/'],
+            bzrdir.BzrDir.find_bzrdirs(smart_transport))
+
     def test_find_bzrdirs_list_current(self):
         def list_current(transport):
             return [s for s in transport.list_dir('') if s != 'baz']
@@ -817,7 +855,6 @@
                                 bzrdir.BzrDir.find_bzrdirs(transport,
                                     list_current=list_current))
 
-
     def test_find_bzrdirs_evaluate(self):
         def evaluate(bzrdir):
             try: