[MERGE] Add --branch option to cmd_root (updated)

[Ian Clatworthy]

Roland Mas wrote:

> Following remarks by Ian Clatworthy, here's my second take at a new
> option --branch for bzr root, with some code reorganisation and unit
> tests.  Rebased on current trunk.

Well done. This is almost ready to merge, assuming a second reviewer
agrees. I've highlighted the tweaks I want done before landing. These
are normally done by the person landing the code but you're welcome to
do them and resubmit as well, saving us time in getting it landed.

bb:tweak

> +    def get_branch_root(self):
> +        """Return the root of the branch in this bzrdir.
>  
> +        :raises NotBranchError: If there is no Branch.
> +        :return: The root of the branch.
> +        """
> +        try:
> +            tree = self.open_workingtree(recommend_upgrade=False)
> +        except (NoWorkingTree, NotLocalUrl):
> +            tree = None
> +            try:
> +                branch = self.open_branch()
> +            except NotBranchError:
> +                branch = None
> +                try:
> +                    repository = self.open_repository()
> +                except NoRepositoryPresent:
> +                    # Return silently; cmd_root already returned NotBranchError
> +                    # if no bzrdir could be opened.
> +                    return
> +                else:
> +                    lockable = repository

I think the whole "except NotBranchError" clause can go. Keep in mind
that this is a method in a library now, so exactly what one client
(cmd_root) wants is less important than making the method make semantic
sense. In this case, the method is called "get_method_root" and the
docstring says it will return NotBranchError if called on a bzrdir
without a branch, so just let that exception propagate accordingly.

> +# Copyright (C) 2006, 2007, 2008 Canonical Ltd
> +# Copyright (C) 2009, Roland Mas

Please just extend the top line to include the current year. All the
code in bzrlib is simply "Copyright Canonical" for legal reasons. The
contributor of various authors to various parts in captured in NEWS and
the revision log.

> +    def test_root_non_existing(self):
> +        if sys.platform == "win32":
> +            location = "C:/i/do/not/exist/"
> +        else:
> +            location = "/i/do/not/exist/"

Is this if statement really needed? I thought Windows would see
"/foo/bar" as implicitly being on the current drive so I don't think
prepending the "C:" bit is required. I could be wrong though.

> +        # Create initial standalone branch
> +        tree1 = self.make_branch_and_tree('standalone', 'weave')

I'm pretty sure the "weave" parameter is unnecessary here.

Otherwise, well done and thanks for the patch!

Ian C.