[merge] Make "bzr info" terse by default

[Andrew Bennetts]

Aaron Bentley wrote:
> Okay, I dug back into this, got tests passing, and added support for
> describing bound branches.  Not proposing it for 0.17, but let's get in
> into early 0.18.

I like this.  I have some quibbles, but I basically really really like this, and
even with those quibbles it's an improvement over what we had and I'm happy to
see this merged.

[...]
> === added file bzrlib/tests/test_info.py // file-id:test_info.py-20070320150933
[...]
> +    def assertTreeDescription(self, format):
> +        self.make_branch_and_tree('%s_tree' % format, format=format)
> +        tree = workingtree.WorkingTree.open('%s_tree' % format)
> +        self.assertEqual(format, info.describe_format(tree.bzrdir,
> +            tree.branch.repository, tree.branch, tree))

A docstring would be nice.  The phrase "assert tree description" isn't quite as
obvious as "assert equal".  The name sounds like it's asserting that a tree has
a description, but you're actually asserting something different (that a tree
with a particular format generates a description that matches the format
string?).  I'm ok with the name, so long as there's a docstring to clarify the
purpose.

Ditto assertCheckoutDescription, etc.

[...]
> +        # this ought to be easier...
> +        branch.create_checkout('%s_co' % format,
> +            lightweight=True).bzrdir.destroy_workingtree()
> +        control = bzrdir.BzrDir.open('%s_co' % format)
> +        old_format = control._format.workingtree_format
> +        try:
> +            control._format.workingtree_format = \
> +                bzrdir.format_registry.make_bzrdir(format).workingtree_format
> +            control.create_workingtree()

It'd be nice if you could just pass a working tree format to
branch.create_checkout (and to control.create_workingtree).  Oh well.

> +    def test_describe_format(self):
> +        for key in bzrdir.format_registry.keys():
> +            if key == 'default':
> +                continue
> +            self.assertTreeDescription(key)
> +
> +        for key in bzrdir.format_registry.keys():
> +            if key in ('default', 'weave'):
> +                continue
> +            expected = None
> +            if key in ('dirstate', 'dirstate-tags', 'dirstate-with-subtree'):
> +                expected = 'dirstate or dirstate-tags'
> +            if key in ('knit', 'metaweave'):
> +                expected = 'knit or metaweave'
> +            self.assertCheckoutDescription(key, expected)
> +
> +        for key in bzrdir.format_registry.keys():
> +            if key == 'default':
> +                continue
> +            expected = None
> +            if key in ('dirstate', 'knit'):
> +                expected = 'dirstate or knit'
> +            self.assertBranchDescription(key, expected)
> +
> +        for key in bzrdir.format_registry.keys():
> +            if key == 'default':
> +                continue
> +            expected = None
> +            if key in ('dirstate', 'knit', 'dirstate-tags'):
> +                expected = 'dirstate or dirstate-tags or knit'
> +            self.assertRepoDescription(key, expected)

These four loops don't depend on each other at all, so I'd probably make them
into separate test methods.  It tends to make debugging more convenient (and
test failures more informative, because they're more specific).

> +        format = bzrdir.format_registry.make_bzrdir('metaweave')
> +        format.set_branch_format(_mod_branch.BzrBranchFormat6())
> +        tree = self.make_branch_and_tree('unknown', format=format)
> +        self.assertEqual('unnamed', info.describe_format(tree.bzrdir,
> +            tree.branch.repository, tree.branch, tree))

You do "info.describe_layout(tree.branch.repository, tree.branch, tree)" a fair
bit in these tests, and also "info.describe_format(...)" with four args.
Perhaps there should be convenience methods for those, that just takes a tree
and fills in the rest of the arguments for you.  Maybe "describe_tree_format" /
"describe_tree_layout"?

It's good that you can call describe_format/layout and directly specify the
control/repo/branch/tree to use, it makes exercising the different combinations
much easier.

> +    def test_gather_location_checkout(self):
> +        tree = self.make_branch_and_tree('tree')
> +        hcheckout = tree.branch.create_checkout('hcheckout')
> +        self.assertEqual(
> +            {'checkout root': hcheckout.bzrdir.root_transport.base,
> +             'checkout of branch': tree.bzrdir.root_transport.base},
> +            info.gather_location_info(hcheckout.branch.repository,
> +                                      hcheckout.branch, hcheckout))

Not sure what the "h" in "hcheckout" stands for.  Oh, "heavyweight"?  That's
just a little bit too terse :)

I'd probably just split this into multiple test methods,
test_gather_location_heavyweight_checkout,
test_gather_location_lightweight_checkout,
test_gather_location_lightweight_checkout_of_heavyweight_checkout, etc.  (Maybe
just "heavy" instead of "heavyweight", but you get the idea).  If you did that,
I think the "hcheckout" and similar would be clear enough.  I don't think the
slightly duplication in setUp would be too bad.

Also, I'm not sure that the change to gather_location_info to return a dict is a
good idea.  A list of 2-tuples would be better, so that the ordering of the
lines in the output stays predictable would be better.

Hmm, reading ahead I see your _show_location_info function ensures the ordering,
but that seems much more complicated than just returning them in the right order
in the first place.  I also don't see any tests for the ordering, but I guess
that was true already...

It also seems prone to mistakes like mismatches between the keys generated by
gather_location_info and _show_location_info.  "bound to branch" for example
appears to be silently ignored.

[...]
> +def describe_format(control, repository, branch, tree):
> +    """Determine the format of an existing control directory
> +
> +    Several candidates may be found.  If so, the names are returned as a
> +    single string, separated by ' or '.
> +
> +    If no matching candidate is found, "unnamed" is returned.
> +    """
> +    candidates  = []

Tiny whitespace nit there.

> +    if len(candidates) == 0:
> +        return 'unnamed'

I wonder if we should give more information than just "unnamed" in this case?
Probably something to worry about later.

-Andrew.