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

Revision history for this message
Brian de Alwis (slyguy) wrote :

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

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
    a) do equivalent repository-probing on a NotBranchError
    b) to serialize the NotBranchError.detail if present
    c) to pick out the NotBranchError.detail on deserialization
   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.

« Back to merge proposal