Bazaar

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

  1. find_bzrdirs-ignore-PermissionDenied-dirs
  2. Merge into bzr.dev
Proposed by Marius Kruger
Status: Merged
Merged at revision: 5320
Proposed branch: lp:~amanica/bzr/find_bzrdirs-ignore-PermissionDenied-dirs
Merge into: lp:bzr
Diff against target: 130 lines (+50/-6) (has conflicts)
4 files modified
NEWS (+6/-0)
bzrlib/bzrdir.py (+2/-3)
bzrlib/tests/__init__.py (+4/-2)
bzrlib/tests/test_bzrdir.py (+38/-1)
Text conflict in NEWS
To merge this branch: bzr merge lp:~amanica/bzr/find_bzrdirs-ignore-PermissionDenied-dirs
Reviewer Review Type Date Requested Status
Robert Collins (community) Needs Fixing
Andrew Bennetts Needs Information
Review via email: mp+24979@code.launchpad.net

Commit message

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

Description of the change

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!).

To post a comment you must log in.
Revision history for this message
Andrew Bennetts (spiv) wrote :
Download full text (4.0 KiB)

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...

Read more...

review: Needs Information
Revision history for this message
Robert Collins (lifeless) wrote :

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

-Rob

Revision history for this message
Robert Collins (lifeless) wrote :

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
Revision history for this message
Marius Kruger (amanica) wrote :

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 ?

Revision history for this message
Robert Collins (lifeless) wrote :

bzrlib.tests.matchers ? :)

Revision history for this message
Robert Collins (lifeless) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2010-05-21 07:17:13 +0000
3+++ NEWS 2010-05-24 20:06:20 +0000
4@@ -97,9 +97,15 @@
5 * ``bzr selftest`` should not use ui.note() since it's not unicode safe.
6 (Vincent Ladeuil, #563997)
7
8+<<<<<<< TREE
9 * CommitBuilder refuses to create revisions whose trees have no root.
10 (Aaron Bentley)
11
12+=======
13+* ``BzrDir.find_bzrdirs`` should ignore dirs that raises PermissionDenied.
14+ (Marius Kruger)
15+
16+>>>>>>> MERGE-SOURCE
17 * Don't mention --no-strict when we just issue the warning about unclean trees.
18 (Vincent Ladeuil, #401599)
19
20
21=== modified file 'bzrlib/bzrdir.py'
22--- bzrlib/bzrdir.py 2010-05-20 18:23:17 +0000
23+++ bzrlib/bzrdir.py 2010-05-24 20:06:20 +0000
24@@ -375,14 +375,14 @@
25 recurse = True
26 try:
27 bzrdir = BzrDir.open_from_transport(current_transport)
28- except errors.NotBranchError:
29+ except (errors.NotBranchError, errors.PermissionDenied):
30 pass
31 else:
32 recurse, value = evaluate(bzrdir)
33 yield value
34 try:
35 subdirs = list_current(current_transport)
36- except errors.NoSuchFile:
37+ except (errors.NoSuchFile, errors.PermissionDenied):
38 continue
39 if recurse:
40 for subdir in sorted(subdirs, reverse=True):
41@@ -1958,7 +1958,6 @@
42 format_string = transport.get_bytes(".bzr/branch-format")
43 except errors.NoSuchFile:
44 raise errors.NotBranchError(path=transport.base)
45-
46 try:
47 return klass._formats[format_string]
48 except KeyError:
49
50=== modified file 'bzrlib/tests/__init__.py'
51--- bzrlib/tests/__init__.py 2010-05-20 18:23:17 +0000
52+++ bzrlib/tests/__init__.py 2010-05-24 20:06:20 +0000
53@@ -2408,9 +2408,11 @@
54 made_control = self.make_bzrdir(relpath, format=format)
55 return made_control.create_repository(shared=shared)
56
57- def make_smart_server(self, path):
58+ def make_smart_server(self, path, backing_server=None):
59+ if backing_server is None:
60+ backing_server = self.get_server()
61 smart_server = test_server.SmartTCPServer_for_testing()
62- self.start_server(smart_server, self.get_server())
63+ self.start_server(smart_server, backing_server)
64 remote_transport = get_transport(smart_server.get_url()).clone(path)
65 return remote_transport
66
67
68=== modified file 'bzrlib/tests/test_bzrdir.py'
69--- bzrlib/tests/test_bzrdir.py 2010-03-25 14:22:41 +0000
70+++ bzrlib/tests/test_bzrdir.py 2010-05-24 20:06:20 +0000
71@@ -54,6 +54,7 @@
72 from bzrlib.transport import (
73 get_transport,
74 memory,
75+ pathfilter,
76 )
77 from bzrlib.transport.http._urllib import HttpTransport_urllib
78 from bzrlib.transport.nosmart import NoSmartTransportDecorator
79@@ -807,6 +808,43 @@
80 self.assertEqualBzrdirs([baz, foo, bar],
81 bzrdir.BzrDir.find_bzrdirs(transport))
82
83+ def make_fake_permission_denied_transport(self, transport, paths):
84+ """Create a transport that raises PermissionDenied for some paths."""
85+ def filter(path):
86+ if path in paths:
87+ raise errors.PermissionDenied(path)
88+ return path
89+ path_filter_server = pathfilter.PathFilteringServer(transport, filter)
90+ path_filter_server.start_server()
91+ self.addCleanup(path_filter_server.stop_server)
92+ path_filter_transport = pathfilter.PathFilteringTransport(
93+ path_filter_server, '.')
94+ return (path_filter_server, path_filter_transport)
95+
96+ def assertBranchUrlsEndsWith(self, expect_url_suffixes, actual_bzrdirs):
97+ """Check that each branch url ends with the corresponding suffix"""
98+ actual_bzrdirs = list(actual_bzrdirs)
99+ self.assertEqual(len(expect_url_suffixes), len(actual_bzrdirs))
100+ for expect_url_suffix, actual_bzrdir in zip(
101+ expect_url_suffixes, actual_bzrdirs):
102+ self.assertEndsWith(actual_bzrdir.user_url, expect_url_suffix)
103+
104+ def test_find_bzrdirs_permission_denied(self):
105+ foo, bar, baz = self.make_foo_bar_baz()
106+ transport = get_transport(self.get_url())
107+ (path_filter_server, path_filter_transport
108+ ) = self.make_fake_permission_denied_transport(transport, ['foo'])
109+
110+ # local transport
111+ self.assertBranchUrlsEndsWith(['baz/'],
112+ bzrdir.BzrDir.find_bzrdirs(path_filter_transport))
113+
114+ # smart server
115+ smart_transport = self.make_smart_server('.',
116+ backing_server=path_filter_server)
117+ self.assertBranchUrlsEndsWith(['baz/'],
118+ bzrdir.BzrDir.find_bzrdirs(smart_transport))
119+
120 def test_find_bzrdirs_list_current(self):
121 def list_current(transport):
122 return [s for s in transport.list_dir('') if s != 'baz']
123@@ -817,7 +855,6 @@
124 bzrdir.BzrDir.find_bzrdirs(transport,
125 list_current=list_current))
126
127-
128 def test_find_bzrdirs_evaluate(self):
129 def evaluate(bzrdir):
130 try: