Code review comment for lp:~slyguy/bzr/bug-440952-bzrdir

Thanks for the speedy review, Aaron.

On 15-Oct-2009, at 3:15 PM, Aaron Bentley wrote:
> This looks like it would cause Branch.open_containing to do twice as
> 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.open_containing is a bit tortuous, but it seems to do its work
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.open_branch()
instead.

Placing the test in the command runner (I assume you mean something
like Command.run_argv_aliases?) is a bit difficult. Although
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://bzr.savannah.nongnu.org/r/man-db
>> http://bzr.savannah.nongnu.org/r/man-db/ is permanently redirected
>> to http+urllib://bzr.savannah.gnu.org/r/man-db/
>> bzr: ERROR: Not a branch: "http+urllib://bzr.savannah.gnu.org/r/man-db/.bzr/branch/
>> ": 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_fallbacks=False):
>> - """See BzrDir.open_branch."""
>> + """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_on_branch_from_repo(self):
>
> ^^^ 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>