[MERGE] Add --branch-root option to cmd_root

[Ian Clatworthy]

Roland Mas wrote:

>   I discovered the "bzr root" command just today.  I had been longing
> for something like that for some time, but apparently I should have read
> the docs more deeply.  Anyway, I came up with this extension, which I
> would like to see reviewed and possibly committed.  It adds a
> --branch-root option to that command, which will return the location of
> the branch in case it's different from the root of the working tree (for
> lightweight checkouts) or in case there's no working tree (for
> standalone branches or branches in a no-trees repository).

Hi Roland,

Thanks for the patch and apologies for the delay in reviewing it. This
is an excellent idea and the implementation looks pretty sound. The main
thing preventing from me from approving it for landing is lack of tests.

bb:resubmit

Here's how to fix that ...

1. Make the new logic a function in bzrlib/bzrdir.py and call that
   function if the new option is defined. I think the function should
   return a value (or list?) and the output of the value (or values?)
   should be left in cmd_root.run().

2. Add some unit tests to bzrlib/tests/test_bzrdir.py testing your new
   function. I'd add a new test class to the end of that file. You
   may find some of the tests in the ChrootedTests class useful for
   copying and pasting into your new tests.

3. Add bzrlib/tests/blackbox/test_root.py containing a simple test
   that the root command can be called with your new option.

4. Register test_root.py in bzrlib/tests/blackbox/__init__.py in the
   appropriate place. (It isn't automatically detected.)

Some other comments ...

I'd prefer the option to be named --of-branch or simply --branch. I
don't think we need to repeat "root" given the command name.

> +            from bzrlib.info import gather_location_info
> +            lockable.lock_read()
> +            locs = gather_location_info(repository, branch, tree)
> +            for loc in locs:
> +                if loc[0] in ('checkout of branch', 'branch root', 'repository branch'):
> +                    path = urlutils.local_path_from_url(loc[1])
> +                    self.outf.write(path + '\n')
> +            lockable.unlock()
> +        else:
> +            self.outf.write(tree.basedir + '\n')

The code after locakable.read() but before lockable.unlock() needs to go
in a try/finally block (with lockable.unlock() in the finally clause).

Also, I suspect you can break out of the for loop as soon as your find a
path. Assuming that whole code block becomes a function as suggested,
simply

  return urlutils.local_path...

ought to do.

I hope all the above makes sense. If you have any questions, please let
us know.

Ian C.