Merge lp:~jameinel/bzr/lolando-branch-root into lp:bzr

I'm submitting this on behalf of Lo-lan-do who doesn't seem to have an LP account, and thus cannot request a merge. (Limitation of our current scheme?)

,----
| Hi all,
|
| Resubmitting a merge request that apparently got lost when Bundle
| Buggy was abandoned. This patch adds a --branch-root option to bzr
| root, which can be used to find the location of a branch even from
| within a lightweight checkout.
`----

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

John A Meinel wrote:
> John A Meinel has proposed merging lp:~jameinel/bzr/lolando-branch-root into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
>
> I'm submitting this on behalf of Lo-lan-do who doesn't seem to have an LP account, and thus cannot request a merge. (Limitation of our current scheme?)
>
> ,----
> | Hi all,
> |
> | Resubmitting a merge request that apparently got lost when Bundle
> | Buggy was abandoned. This patch adds a --branch-root option to bzr
> | root, which can be used to find the location of a branch even from
> | within a lightweight checkout.
> `----
>

Note that AFAIK, Lo-lan-do has not yet signed the
http://www.canonical.com/contributors
agreement. Also I haven't formally reviewed this, so there shouldn't be
an implicit +1 from myself.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkrE8NgACgkQJdeBCYSNAANZKwCgpg2/cEVX8Bpbu9lbQNE9EwRW
yGwAoLGDJ7cYDKYWOp7j7VguGgACwrgM
=ABjE
-----END PGP SIGNATURE-----

It's not super elegant and I'd like to see at least a smoke test, but it otherwise seems ok.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Martin Pool wrote:
> It's not super elegant and I'd like to see at least a smoke test, but it otherwise seems ok.

Actually if I'm reading the diff correctly it seems quite broken when
you *don't* supply the option.

specifically:

+ def run(self, filename=None, branch_root=False):
         """Print the branch root."""
- - tree = WorkingTree.open_containing(filename)[0]
- - self.outf.write(tree.basedir + '\n')
+ if branch_root:

^- This seems to remove the code that opens the tree
...

+ else:
+ self.outf.write(tree.basedir + '\n')
+

^- This directly accesses 'tree.basedir' in the else clause, but the
tree hasn't been opened.

 review: needs_fixing

I also don't understand why it goes through
'info.gather_location_info()' and then sort of parses the text there.
Maybe to handle stuff like heavyweight checkouts?

I think the direct way is:

a_bzrdir = bzrdir.BzrDir.open_containing(...)
reference = a_bzrdir.get_branch_reference()
if reference is not None:
  # This is a lightweight checkout of some other location
else:
  branch = a_bzrdir.open_branch()

However, open_branch() will open the referenced branch, so it may just be:

tree = WorkingTree.open_containing('.')[0]
if branch_root:
  self.outf.write(tree.branch.base)
else:
  self.outf.write(tree.basedir)

I agree with Martin that we should have at least a smoke test of this in
bzrlib/tests/blackbox/test_root.py

Though it is possible we don't have direct tests of "bzr root" yet.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkrE9P8ACgkQJdeBCYSNAAPu/ACfW3usaFT49dWK409uxqI/CkdI
ZcwAn0aFdLWkUL1IcGrdgsHD8zeE7Ry6
=Kavs
-----END PGP SIGNATURE-----