Code review comment for lp:~jameinel/bzr/lolando-branch-root

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