Merge lp:~slyguy/bzr/bug-440952-bzrdir into lp:bzr
- bug-440952-bzrdir
- Merge into bzr.dev
Status: | Merged |
---|---|
Merged at revision: | not available |
Proposed branch: | lp:~slyguy/bzr/bug-440952-bzrdir |
Merge into: | lp:bzr |
Diff against target: |
219 lines 6 files modified
bzrlib/bzrdir.py (+48/-7) bzrlib/errors.py (+6/-3) bzrlib/remote.py (+3/-0) bzrlib/smart/bzrdir.py (+12/-6) bzrlib/tests/blackbox/test_shared_repository.py (+9/-0) bzrlib/tests/test_smart.py (+16/-0) |
To merge this branch: | bzr merge lp:~slyguy/bzr/bug-440952-bzrdir |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Robert Collins (community) | Needs Fixing | ||
Aaron Bentley (community) | Needs Fixing | ||
Review via email: mp+13431@code.launchpad.net |
Commit message
Description of the change
Brian de Alwis (slyguy) wrote : | # |
Aaron Bentley (abentley) wrote : | # |
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
review needs-fixing
Brian de Alwis wrote:
> Brian de Alwis has proposed merging lp:~slyguy/bzr/bug-440952-bzrdir into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
>
> This branch contains an attempt to tackle bug 440952 (https:/
>
> Following a suggestion from John, I've modified the BzrDir.
This looks like it would cause Branch.
many probes. Is that true?
Ideally, we would do this probe at the highest level, since
NotBranchError isn't always user-visible. It would be better to do it
in Branch.open, or maybe even by catching NotBranchError in the command
runner and testing for repositories there.
> $ ./bzr branch http://
> http://
> bzr: ERROR: Not a branch: "http+urllib:
>
> This change is broken into 4 parts:
>
> 1) Extend NotBranchError to hold an extra field, "detail". If present, this field provides a meaningful explanation to the user why the location is not a branch. I couldn't use the existing "extra" field as the superclass uses it to hold a nicely-formatted variant of whatever is passed (e.g., it prefixes whatever is provided with ": ").
>
> 2) There are many variants of BzrDir, but as pointed out by John, the only one that actually seemed to need modification is BzrDirMeta1. We modify open_branch() and get_branch_
>
> 3) [This is the sketchiest part of my solution.] I've modified the HPSS to
> a) do equivalent repository-probing on a NotBranchError
> b) to serialize the NotBranchError.
> c) to pick out the NotBranchError.
> I'm not sure if there is a better way.
>
> 4) I've added tests to ensure the detail is reported when attempting to branch from a repository to bzrlib/
>
>
> @@ -1465,7 +1478,11 @@
> return not isinstance(
>
> def open_branch(self, unsup...
Brian de Alwis (slyguy) wrote : | # |
Thanks for the speedy review, Aaron.
On 15-Oct-2009, at 3:15 PM, Aaron Bentley wrote:
> This looks like it would cause Branch.
> many probes. Is that true?
So not that I've seen: to experiment, I placed a mutter in the
exception code and then tried running various bzr commands from
various locations to affect various locations. Basically the code is
only invoked whenever I specify a location that is a repository — I
guess most commands must have something that filters out non-.bzr
containing locations.
Branch.
using transport objects, and doesn't seem to use BzrDir.open_branch.
> Ideally, we would do this probe at the highest level, since
> NotBranchError isn't always user-visible. It would be better to do it
> in Branch.open, or maybe even by catching NotBranchError in the
> command
> runner and testing for repositories there.
So attempting to restrict changes to Branch.open doesn't seem to be
possible as there is a lot of the code that uses BzrDir.
instead.
Placing the test in the command runner (I assume you mean something
like Command.
NotBranchError.path provides the path/URL, it isn't necessarily the
path actually provided by the user. The example from the motivating
use case demonstrates this: the exception reports the path as ".../man-
db/.bzr/branch":
>> $ ./bzr branch http://
>> http://
>> to http+urllib:
>> bzr: ERROR: Not a branch: "http+urllib:
>> ": location is a repository.
Finally, I think there are two compelling reasons to recommend such a
test happen as close to the error as possible. First, it avoids an
additional probe when using HPSS. Second, it's entirely possible that
there will be some foreign VCS that won't have a concept of a
repository; bzr-svn, for example, provides its own BzrDir subclass and
will never perform the has_repository tests.
Re: your code comments:
>> def open_branch(self, unsupported=False, ignore_
>> - """See BzrDir.
>> + """See BzrDir.open_branch.
>> +
>> + Note: Test for repository on failure is not done here as
>> + BzrDirPreSplitOut cannot have a repository.
>
> This is false: BzrDirPreSplitOut *always* has a repository, and it
> *always* has a branch. There's no point testing for a repository
> because if there's no branch, there's no BzrDirPreSplitOut and no
> repository.
I've amended the comment.
>> + def test_notif_
>
> ^^^ typo
And changed all tests to use consistent method names.
I've updated the code. I hope this addresses your comments.
Brian.
--
Brian de Alwis
<email address hidden>
Andrew Bennetts (spiv) wrote : | # |
Overall, this is a good patch, and remarkably thorough. Thank you!
I like the approach that only looks for the repository in the error case. I'm a
little concerned that there may be a case that calls open_containing or
something that will get the NotBranchError, do the extra lookup, then ignore the
detail because it just tries the parent directory. I think that's not very
common, and at worst it will only happen for every BzrDir found in while walking
parent dirs. So I think that's ok. Perhaps if we wanted to be really fancy
we'd actually make NotBranchError.
looks at it or stringifies the error, we don't pay the cost, but I'm ok with
worrying about that later (if at all).
Because Aaron's already reviewed this I've focused my comments on the HPSS code,
which he didn't really comment on.
Finally, I realise we're asking for a fair bit extra work on top of the
obviously large amount you've already done. If you don't feel able to do this
work soon let us know. I'm happy to finish this work if you're too busy,
although if you can do it soon I'm sure it will get done sooner than if I add it
my todo list :)
Brian de Alwis wrote:
[...]
> 3) [This is the sketchiest part of my solution.] I've modified the HPSS to
> a) do equivalent repository-probing on a NotBranchError
> b) to serialize the NotBranchError.
> c) to pick out the NotBranchError.
> I'm not sure if there is a better way.
I think that's a reasonable approach. I suspect eventually we'll get to the
point where we simply do a single HPSS call that asks for the details of all of
BzrDir, Repository, Branch (and maybe WorkingTree) that are present at a
location... if we did that then we'd simply infer this situation from the result
of that rather than NotBranchError. But until then I think this is a simple and
effective solution.
> 4) I've added tests to ensure the detail is reported when attempting to
> branch from a repository to bzrlib/
> and bzrlib/
> the best locations, but they seemed to have similar style tests.
Not the best, but a great start! I've given some suggestions below.
[...]
> def get_branch_
> """See BzrDir.
> - from bzrlib.branch import BranchFormat
> - format = BranchFormat.
> - return format.
> + try:
> + from bzrlib.branch import BranchFormat
> + format = BranchFormat.
> + return format.
> + except errors.
> + # provide indication to the user if they're actually
> + # opening a repository (see bug 440952)
> + if self.has_
> + raise errors.
> + raise e
I wonder if it would be better to do:
if e.detail is None and self.has_
e.detail = 'location is a repository'
raise e
I think i...
Brian de Alwis (slyguy) wrote : | # |
On 16-Oct-2009, at 1:48 AM, Andrew Bennetts wrote:
> Finally, I realise we're asking for a fair bit extra work on top of
> the
> obviously large amount you've already done. If you don't feel able
> to do this
> work soon let us know. I'm happy to finish this work if you're too
> busy,
> although if you can do it soon I'm sure it will get done sooner than
> if I add it
> my todo list :)
Since most of the extra work are a bit beyond my ken, if you wouldn't
mind taking it on, I would be most appreciative if you took it on :-)
Thanks Andrew!
Brian.
Aaron Bentley (abentley) wrote : | # |
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
review needs-fixing
Brian de Alwis wrote:
> Branch.
> using transport objects, and doesn't seem to use BzrDir.open_branch.
It absolutely does. First it acquires a BzrDir using
BzrDir.
BzrDir.open_branch.
>> Ideally, we would do this probe at the highest level, since
>> NotBranchError isn't always user-visible. It would be better to do it
>> in Branch.open, or maybe even by catching NotBranchError in the
>> command
>> runner and testing for repositories there.
>
> So attempting to restrict changes to Branch.open doesn't seem to be
> possible as there is a lot of the code that uses BzrDir.
> instead.
I didn't mean that. I meant that ideally we would only probe for a
repository when we know that NotBranchError will be displayed to the
user, and when we know that the location was user-entered.
> Placing the test in the command runner (I assume you mean something
> like Command.
> NotBranchError.path provides the path/URL, it isn't necessarily the
> path actually provided by the user.
Why is that relevant? When you probe for the repository, you're not
using the path entered by the user anyhow.
> Finally, I think there are two compelling reasons to recommend such a
> test happen as close to the error as possible. First, it avoids an
> additional probe when using HPSS.
I think that a single additional probe will not be visible to users. If
there is a slight delay introduced, remember that it will only happen
when there is a error is displayed to users.
Your proposed change will cause additional probes to happen in every
case. On the local filesystem, the low-latency IO means that they will
not be observable, and on HPSS, the probes will be does as part of a
single operation, so they will not cause additional roundtrips. But
over dumb transports such as http and sftp, they can cause observable
lag. Many times, NotBranchErrors are caught and handled, and when they
are, these probes are unnecessary. It's possible for many
NotBranchErrors may be encountered during a successful bzr run.
To summarize: your change can cause multiple, observable probes on dumb
transports, but avoids an additional probe on smart transports when
displaying the error to the user. My suggestion would cause a single
additional probe on smart transports when displaying an error to the
user. I think that is better from the perspective of minimizing probes.
> Second, it's entirely possible that
> there will be some foreign VCS that won't have a concept of a
> repository; bzr-svn, for example, provides its own BzrDir subclass and
> will never perform the has_repository tests.
I think what you're trying to say here is that we would reopen the
BzrDir unnecessarily in that case? We could certainly attach the BzrDir
to the NotBranchError to avoid that. But again, slightly more lag
during error handling isn't the end of the world.
> === modified file 'bzrlib/bzrdir.py'
> --- bzrlib/bzrdir.py 2009-09-24 09:45:23 +0000
>...
Robert Collins (lifeless) wrote : | # |
On Thu, 2009-10-15 at 17:05 +0000, Brian de Alwis wrote:
> Brian de Alwis has proposed merging lp:~slyguy/bzr/bug-440952-bzrdir into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
>
> This branch contains an attempt to tackle bug 440952 (https:/
I see you already have some excellent reviews. I wonder perhaps if
changing open_branch/
complexity it adds? It makes a clearly defined and *predictable*
overhead function sometimes twice as expensive. And the function can be
a good fraction of a second.
I'd rather not see 'has_repository' as a public method. I don't see
*bzr* repo's suffering from many extra probes - bzrdirs without branches
that are not repositories are pretty rare. However, the svn mapping
creates a svndir object at every path under an svn repo and at every dir
in an svn tree, which we then loop up on - so the performance impact
there could be substantial.
So in balance, I'd also rather see this done from the command runner or
Branch.open. For the latter, just putting the bzrdir that was found *if*
one was found in NotBranchError would let
Branch.
the core logic to be unpredictable. Doing it from the command runner is
probably too high up.
review: needsfixing
Andrew Bennetts (spiv) wrote : | # |
Also, Brian, can you complete a contributor agreement? <http://
Thanks very much.
Brian de Alwis (slyguy) wrote : | # |
On 17-Nov-2009, at 8:01 PM, Andrew Bennetts wrote:
> Also, Brian, can you complete a contributor agreement? <http://
Done. I thought I had done this previously, but can't find any record of having sent it.
Vincent Ladeuil (vila) wrote : | # |
Brian, Andrew, can you update this merge proposal so we know who is working on what ?
Martin Pool (mbp) wrote : | # |
This seems a bit stalled because there is no clear next step. It looks like Andrew has offered to take it from here, which slyguy has accepted. Also, Robert has some additional comments. Since Andrew's patch pilot for the first week of 2010, I'm going to set this back to work in progress, and mark 440952 as in progress and assigned to him, and hopefully he can take it from there.
Preview Diff
1 | === modified file 'bzrlib/bzrdir.py' |
2 | --- bzrlib/bzrdir.py 2009-09-24 09:45:23 +0000 |
3 | +++ bzrlib/bzrdir.py 2009-10-15 20:21:14 +0000 |
4 | @@ -1034,6 +1034,19 @@ |
5 | except errors.NotBranchError: |
6 | return False |
7 | |
8 | + def has_repository(self): |
9 | + """Tell if this bzrdir contains a repository. |
10 | + |
11 | + Note: if you're going to open the repository, you should just go ahead |
12 | + and try, and not ask permission first. (This method just opens the |
13 | + repository and discards it, and that's somewhat expensive.) |
14 | + """ |
15 | + try: |
16 | + self.open_repository() |
17 | + return True |
18 | + except errors.NoRepositoryPresent: |
19 | + return False |
20 | + |
21 | def has_workingtree(self): |
22 | """Tell if this bzrdir contains a working tree. |
23 | |
24 | @@ -1465,7 +1478,12 @@ |
25 | return not isinstance(self._format, format.__class__) |
26 | |
27 | def open_branch(self, unsupported=False, ignore_fallbacks=False): |
28 | - """See BzrDir.open_branch.""" |
29 | + """See BzrDir.open_branch. |
30 | + |
31 | + Note: Test for repository on failure is not required here as |
32 | + BzrDirPreSplitOut always has a branch; if there's no branch, |
33 | + then there's no BzrDirPreSplitOut and no repository. |
34 | + """ |
35 | from bzrlib.branch import BzrBranchFormat4 |
36 | format = BzrBranchFormat4() |
37 | self._check_supported(format, unsupported) |
38 | @@ -1642,9 +1660,16 @@ |
39 | |
40 | def get_branch_reference(self): |
41 | """See BzrDir.get_branch_reference().""" |
42 | - from bzrlib.branch import BranchFormat |
43 | - format = BranchFormat.find_format(self) |
44 | - return format.get_reference(self) |
45 | + try: |
46 | + from bzrlib.branch import BranchFormat |
47 | + format = BranchFormat.find_format(self) |
48 | + return format.get_reference(self) |
49 | + except errors.NotBranchError, e: |
50 | + # provide indication to the user if they're actually |
51 | + # opening a repository (see bug 440952) |
52 | + if self.has_repository(): |
53 | + raise errors.NotBranchError(path=e.path, detail='location is a repository') |
54 | + raise e |
55 | |
56 | def get_branch_transport(self, branch_format): |
57 | """See BzrDir.get_branch_transport().""" |
58 | @@ -1690,6 +1715,15 @@ |
59 | pass |
60 | return self.transport.clone('checkout') |
61 | |
62 | + def has_repository(self): |
63 | + """See BzrDir.has_repository().""" |
64 | + from bzrlib.repository import RepositoryFormat |
65 | + try: |
66 | + RepositoryFormat.find_format(self) |
67 | + return True |
68 | + except errors.NoRepositoryPresent: |
69 | + return False |
70 | + |
71 | def has_workingtree(self): |
72 | """Tell if this bzrdir contains a working tree. |
73 | |
74 | @@ -1743,9 +1777,16 @@ |
75 | |
76 | def open_branch(self, unsupported=False, ignore_fallbacks=False): |
77 | """See BzrDir.open_branch.""" |
78 | - format = self.find_branch_format() |
79 | - self._check_supported(format, unsupported) |
80 | - return format.open(self, _found=True, ignore_fallbacks=ignore_fallbacks) |
81 | + try: |
82 | + format = self.find_branch_format() |
83 | + self._check_supported(format, unsupported) |
84 | + return format.open(self, _found=True, ignore_fallbacks=ignore_fallbacks) |
85 | + except errors.NotBranchError, e: |
86 | + # provide indication to the user if they're actually |
87 | + # opening a repository (see bug 440952) |
88 | + if self.has_repository(): |
89 | + raise errors.NotBranchError(path=e.path, detail='location is a repository') |
90 | + raise e |
91 | |
92 | def open_repository(self, unsupported=False): |
93 | """See BzrDir.open_repository.""" |
94 | |
95 | === modified file 'bzrlib/errors.py' |
96 | --- bzrlib/errors.py 2009-08-30 21:34:42 +0000 |
97 | +++ bzrlib/errors.py 2009-10-15 20:21:14 +0000 |
98 | @@ -702,11 +702,14 @@ |
99 | # TODO: Probably this behavior of should be a common superclass |
100 | class NotBranchError(PathError): |
101 | |
102 | - _fmt = 'Not a branch: "%(path)s".' |
103 | + _fmt = 'Not a branch: "%(path)s"%(extra)s.' |
104 | |
105 | - def __init__(self, path): |
106 | + def __init__(self, path, detail=None): |
107 | import bzrlib.urlutils as urlutils |
108 | - self.path = urlutils.unescape_for_display(path, 'ascii') |
109 | + path = urlutils.unescape_for_display(path, 'ascii') |
110 | + # remember the detail in case of remote serialization |
111 | + self.detail = detail |
112 | + PathError.__init__(self, path=path, extra=self.detail) |
113 | |
114 | |
115 | class NoSubmitBranch(PathError): |
116 | |
117 | === modified file 'bzrlib/remote.py' |
118 | --- bzrlib/remote.py 2009-10-15 04:01:26 +0000 |
119 | +++ bzrlib/remote.py 2009-10-15 20:21:14 +0000 |
120 | @@ -2814,6 +2814,9 @@ |
121 | raise NoSuchRevision(find('repository'), err.error_args[0]) |
122 | elif err.error_tuple == ('nobranch',): |
123 | raise errors.NotBranchError(path=find('bzrdir').root_transport.base) |
124 | + elif err.error_verb == 'nobranch': |
125 | + raise errors.NotBranchError(path=find('bzrdir').root_transport.base, |
126 | + detail=err.error_args[0]) |
127 | elif err.error_verb == 'norepository': |
128 | raise errors.NoRepositoryPresent(find('bzrdir')) |
129 | elif err.error_verb == 'LockContention': |
130 | |
131 | === modified file 'bzrlib/smart/bzrdir.py' |
132 | --- bzrlib/smart/bzrdir.py 2009-09-22 00:34:10 +0000 |
133 | +++ bzrlib/smart/bzrdir.py 2009-10-15 20:21:14 +0000 |
134 | @@ -88,8 +88,10 @@ |
135 | try: |
136 | self._bzrdir = BzrDir.open_from_transport( |
137 | self.transport_from_client_path(path)) |
138 | - except errors.NotBranchError: |
139 | - return FailedSmartServerResponse(('nobranch', )) |
140 | + except errors.NotBranchError, e: |
141 | + if e.detail is None: |
142 | + return FailedSmartServerResponse(('nobranch',)) |
143 | + return FailedSmartServerResponse(('nobranch', e.detail)) |
144 | return self.do_bzrdir_request(*args) |
145 | |
146 | def _boolean_to_yes_no(self, a_boolean): |
147 | @@ -465,8 +467,10 @@ |
148 | return SuccessfulSmartServerResponse(('ok', '')) |
149 | else: |
150 | return SuccessfulSmartServerResponse(('ok', reference_url)) |
151 | - except errors.NotBranchError: |
152 | - return FailedSmartServerResponse(('nobranch', )) |
153 | + except errors.NotBranchError, e: |
154 | + if e.detail is None: |
155 | + return FailedSmartServerResponse(('nobranch',)) |
156 | + return FailedSmartServerResponse(('nobranch', e.detail)) |
157 | |
158 | |
159 | class SmartServerRequestOpenBranchV2(SmartServerRequestBzrDir): |
160 | @@ -481,5 +485,7 @@ |
161 | return SuccessfulSmartServerResponse(('branch', format)) |
162 | else: |
163 | return SuccessfulSmartServerResponse(('ref', reference_url)) |
164 | - except errors.NotBranchError: |
165 | - return FailedSmartServerResponse(('nobranch', )) |
166 | + except errors.NotBranchError, e: |
167 | + if e.detail is None: |
168 | + return FailedSmartServerResponse(('nobranch',)) |
169 | + return FailedSmartServerResponse(('nobranch', e.detail)) |
170 | |
171 | === modified file 'bzrlib/tests/blackbox/test_shared_repository.py' |
172 | --- bzrlib/tests/blackbox/test_shared_repository.py 2009-09-23 23:58:10 +0000 |
173 | +++ bzrlib/tests/blackbox/test_shared_repository.py 2009-10-15 20:21:14 +0000 |
174 | @@ -120,3 +120,12 @@ |
175 | # become necessary for this use case. Please do not adjust this number |
176 | # upwards without agreement from bzr's network support maintainers. |
177 | self.assertLength(15, self.hpss_calls) |
178 | + |
179 | + def test_notification_on_branch_from_repository(self): |
180 | + out, err = self.run_bzr("init-repository -q a") |
181 | + self.assertEqual(out, "") |
182 | + self.assertEqual(err, "") |
183 | + dir = BzrDir.open('a') |
184 | + self.assertIs(dir.has_repository(), True) |
185 | + e = self.assertRaises(errors.NotBranchError, dir.open_branch) |
186 | + self.assertEqual(e.detail, "location is a repository") |
187 | |
188 | === modified file 'bzrlib/tests/test_smart.py' |
189 | --- bzrlib/tests/test_smart.py 2009-09-24 05:31:23 +0000 |
190 | +++ bzrlib/tests/test_smart.py 2009-10-15 20:21:14 +0000 |
191 | @@ -511,6 +511,14 @@ |
192 | self.assertEqual(SmartServerResponse(('ok', reference_url)), |
193 | request.execute('reference')) |
194 | |
195 | + def test_notification_on_branch_from_repository(self): |
196 | + """When there is a repository, the error should return details.""" |
197 | + backing = self.get_transport() |
198 | + request = smart.bzrdir.SmartServerRequestOpenBranch(backing) |
199 | + repo = self.make_repository('.') |
200 | + self.assertEqual(SmartServerResponse(('nobranch', 'location is a repository')), |
201 | + request.execute('')) |
202 | + |
203 | |
204 | class TestSmartServerRequestOpenBranchV2(TestCaseWithChrootedTransport): |
205 | |
206 | @@ -562,6 +570,14 @@ |
207 | response) |
208 | self.assertLength(1, opened_branches) |
209 | |
210 | + def test_notification_on_branch_from_repository(self): |
211 | + """When there is a repository, the error should return details.""" |
212 | + backing = self.get_transport() |
213 | + request = smart.bzrdir.SmartServerRequestOpenBranchV2(backing) |
214 | + repo = self.make_repository('.') |
215 | + self.assertEqual(SmartServerResponse(('nobranch', 'location is a repository')), |
216 | + request.execute('')) |
217 | + |
218 | |
219 | class TestSmartServerRequestRevisionHistory(tests.TestCaseWithMemoryTransport): |
220 |
This branch contains an attempt to tackle bug 440952 (https:/ /bugs.launchpad .net/bugs/ 440952). This bug is based on a complaint where a user tried to branch from a repository directory. Bzr's error message was pretty opaque: the problem was that the location provided was a repository, not a branch.
Following a suggestion from John, I've modified the BzrDir. open_branch( ) and BzrDir. get_branch_ reference( ) to, upon a NotBranchError, do a further probe to see if the location may actually be a repository. If it is a repository, the NotBranchError includes additional detail "location is a repository", which will be visible to the user when reported. For example:
$ ./bzr branch http:// bzr.savannah. nongnu. org/r/man- db bzr.savannah. nongnu. org/r/man- db/ is permanently redirected to http+urllib: //bzr.savannah. gnu.org/ r/man-db/ //bzr.savannah. gnu.org/ r/man-db/ .bzr/branch/ ": location is a repository.
http://
bzr: ERROR: Not a branch: "http+urllib:
This change is broken into 4 parts:
1) Extend NotBranchError to hold an extra field, "detail". If present, this field provides a meaningful explanation to the user why the location is not a branch. I couldn't use the existing "extra" field as the superclass uses it to hold a nicely-formatted variant of whatever is passed (e.g., it prefixes whatever is provided with ": ").
2) There are many variants of BzrDir, but as pointed out by John, the only one that actually seemed to need modification is BzrDirMeta1. We modify open_branch() and get_branch_ reference( ) to catch the NotBranchError and do a further probe. To simplify the code, I've introduced BzrDir. has_repository( ); its default implementation calls open_repository() and returns true if successful, and false if it fails. bzr-svn's SvnRemoteAccess subclasses BzrDir directly, and so should be insulated from this change.
3) [This is the sketchiest part of my solution.] I've modified the HPSS to detail if present detail on deserialization
a) do equivalent repository-probing on a NotBranchError
b) to serialize the NotBranchError.
c) to pick out the NotBranchError.
I'm not sure if there is a better way.
4) I've added tests to ensure the detail is reported when attempting to branch from a repository to bzrlib/ tests/blackbox/ test_shared_ repository. py and bzrlib/ tests/test_ smart.py. I'm not positive that these locations are the best locations, but they seemed to have similar style tests.