Merge lp:~nmb/bzr/893470-dedupe-list-branches into lp:bzr

[Neil Martinsen-Burrell]

This checks to see if the current default branch is one of the colocated branches before returning it in BzrDir.list_branches. This avoids trivial duplication in the output of BzrDir.list_branches().

[Jelmer Vernooij (jelmer)]

Hi Neil,

If you care about duplicates in BzrDir.list_branches(), I think we should just fix that API.

This fix eliminates duplicates - but only for the default branch. We should be consistent and remove dupes for the other branches as well, if we're doing that.

An alternative fix for this (rather than checking afterwards) might be to just have a way to exclude all branch references.

[Jelmer Vernooij (jelmer)]

review: Needs Fixing

[Martin Pool (mbp)]

based on jelmer's comments i'm going to bump this out of the review queue, but if you want to talk about it more, please do.

[Neil Martinsen-Burrell (nmb)]

I would like some guidance on how to put this in a state that could fix bug 893470. In a version of bzr-colo designed to work with the colocated functionality, I always see things like

nmb@garble[~/tmp/test]$ bzr branches
 another
 trunk
 new
 another

Should we be unique-ifying the results of list_branches at any point or should callers have to manage that? Should we be excluding all branch references? I can do either of those, but I'd rather not do anything without knowing that it has a chance of being accepted. The attached patch works for the situation of a colocated workspace with a working tree.

My philosophical belief is that we should only show "unique" branches in some sense when we do list_branches. I think that this should mean that references to other branches already in the list should not be listed again. I don't know enough about BranchReferences to understand if they can refer to branches under a different BzrDir, but if they can, then I think that such branch references should be listed by list_branches.

[Jelmer Vernooij (jelmer)]

Am 07/12/11 06:31, schrieb Neil Martinsen-Burrell:
> I would like some guidance on how to put this in a state that could fix bug 893470. In a version of bzr-colo designed to work with the colocated functionality, I always see things like
>
> nmb@garble[~/tmp/test]$ bzr branches
> another
> trunk
> new
> another
>
> Should we be unique-ifying the results of list_branches at any point or should callers have to manage that? Should we be excluding all branch references? I can do either of those, but I'd rather not do anything without knowing that it has a chance of being accepted. The attached patch works for the situation of a colocated workspace with a working tree.
>
> My philosophical belief is that we should only show "unique" branches in some sense when we do list_branches. I think that this should mean that references to other branches already in the list should not be listed again. I don't know enough about BranchReferences to understand if they can refer to branches under a different BzrDir, but if they can, then I think that such branch references should be listed by list_branches.
There can be branch references to branches outside of the colocated
directory in there as well. I don't think we should be excluding branch
references, as you do want to be able to access them in some situations
- excluding them means there is no way to get at them unless you know
their name.

BzrDir.list_branches() has been available in 2.4 as well, and has always
included branch references so I think we should keep it like that and
instead perhaps add a new method. The most sensible thing (I think)
would be to add a call that returns a dictionary, e.g.:

BzrDir.get_branches() -> { None: Branch("/path/to/some/branch"),
"trunk": Branch("/path/to/some/branch"), "branchb":
Branch("/path/to/some/other/branch"), "bzr.dev":
Branch("http://bazaar-vcs.org/bzr/bzr.dev") }

What do you think?

Cheers,

Jelmer

[Neil Martinsen-Burrell (nmb)]

I agree that we should add a new method and not change the semantics of list_branches. I think a dictionary is a natural object to return. How would the dictionary deal with branch references? That seems like it would give multiple branch objects with the same key. Should the items in the dictionary be lists when this occurs? Should they all be lists, or just those items with multiple objects with that name? For example, what should be the result for the following test:

def test_get_branches(self):
        # Create a branch branch-1 that initially is a checkout of 'foo'
        # Use switch to create 'anotherbranch' which derives from that
        repo = self.make_repository('branch-1', format='development-colo')
        target_branch = repo.bzrdir.create_branch(name='foo')
        branch.BranchReferenceFormat().initialize(
            repo.bzrdir, target_branch=target_branch)
        tree = repo.bzrdir.create_workingtree()

repo.bzrdir.get_branches() -> {'foo':???}

[Jelmer Vernooij (jelmer)]

Am 08/12/11 07:02, schrieb Neil Martinsen-Burrell:
> I agree that we should add a new method and not change the semantics of list_branches. I think a dictionary is a natural object to return. How would the dictionary deal with branch references? That seems like it would give multiple branch objects with the same key. Should the items in the dictionary be lists when this occurs? Should they all be lists, or just those items with multiple objects with that name? For example, what should be the result for the following test:
>
> def test_get_branches(self):
> # Create a branch branch-1 that initially is a checkout of 'foo'
> # Use switch to create 'anotherbranch' which derives from that
> repo = self.make_repository('branch-1', format='development-colo')
> target_branch = repo.bzrdir.create_branch(name='foo')
> branch.BranchReferenceFormat().initialize(
> repo.bzrdir, target_branch=target_branch)
> tree = repo.bzrdir.create_workingtree()
>
> repo.bzrdir.get_branches() -> {'foo':???}
{'foo': Branch("/path/to/bzrdir,branch=foo"), None:
Branch("/path/to/bzrdir,branch=foo")}

IOW, I think we should just open the branch as .open_branch() would.

Cheers,

Jelmer

[Neil Martinsen-Burrell (nmb)]

I have now added a get_branches method to ControlDir and BzrDir, along with tests and a mention in the release notes. I have NOT started using this method in the "bzr branches" command since we currently don't create checkouts when creating development-colo workspaces. When we do, we will have duplicate entries in "bzr branches" and we should migrate it over to using get_branches instead of list_branches.

[Jelmer Vernooij (jelmer)]

Thanks, that looks reasonable.

It seems simpler to just use .open_branch() in ControlDir.get_branches() rather than list_branches(), which doesn't necessarily return the branch with the name None as first element.

I wonder if we should deprecate list_branches now that we have get_branches? Is there any reason to still use list_branches?

75 + def test_get_branches(self):
76 + repo = self.make_repository('branch-1')
77 + target_branch = repo.bzrdir.create_branch(name='foo')
78 + reference = branch.BranchReferenceFormat().initialize(
79 + repo.bzrdir, target_branch=target_branch)
^^ These tests are not bzr-specific, they can be run against e.g. svn as well, where BranchReferenceFormat won't work. To test references, we should instead add (another) test in bzrlib/tests/per_bzrdir.

Cheers,

Jelmer

[Jelmer Vernooij (jelmer)]

We also need a test in bzrlib/tests/per_controldir_colo/test_unsupported.py to make sure that formats without colocated branch support work properly.

review: Needs Fixing

[Jelmer Vernooij (jelmer)]

Perhaps for now we shouldn't deprecate .list_branches() but rather just provide a simple implementation of it tthat just returns self.get_branches().values().

[Neil Martinsen-Burrell (nmb)]

I put the BranchReference test in per_bzrdir/bzrdir.py, leaving a test with a single named branch in per_controldir_colo/test_supported.py. I added a test to per_controldir_colo/test_unsupported.py. I reimplmented controldir.list_branches as get_branches().values() and fixed the tests that depended on a particular order of output from "bzr branches".

[Jelmer Vernooij (jelmer)]

Looks good to me.

One minor nit (can be fixed while landing):

In get_branches(), there is some indentation that is only 3 spaces (should be four), and there should be a space after "None:".

[Jelmer Vernooij (jelmer)]

review: Approve

[Vincent Ladeuil (vila)]

146 + branches themselves. Branches without names use the key None.
147 + (Neil Martinsen-Burrell)

[tweak]
Only the default branch has no name right ? The plural form above is misleading then and mentioning that here and in the doc string will make it clear.

[tweak]
88 + self.assertEqual(set(repo.bzrdir.get_branches().keys()),
89 + set([None, 'foo']))

101 + self.assertEqual(repo.bzrdir.get_branches().keys(), [None])

118 + self.assertEqual(repo.bzrdir.get_branches().keys(),
119 + ['foo'])
120 + self.assertEqual(repo.bzrdir.get_branches()['foo'].base,
121 + target_branch.base)

134 + self.assertEqual(made_control.get_branches().keys(),
135 + [None])

The assertXX(expected, actual) is the preferred idiom. Being coherent here helps when test fail as you don't need to look at the test source code to know which is which especially when a diff between the expected and actual values is displayed. Too bad, it's not documented as such in doc/developers/testing.txt :-/

Both of this tweaks can be done while landing.

review: Needs Fixing

=== modified file 'bzrlib/bzrdir.py'
--- bzrlib/bzrdir.py	2011-12-11 03:44:06 +0000
+++ bzrlib/bzrdir.py	2011-12-11 04:20:30 +0000
@@ -1054,18 +1054,16 @@
                 self.control_files.unlock()
         self.transport.delete_tree(path)
 
-    def list_branches(self):
-        """See ControlDir.list_branches."""
-        ret = []
-        # Default branch
+    def get_branches(self):
+        """See ControlDir.get_branches."""
+        ret = {}
         try:
-            ret.append(self.open_branch())
+            ret[None] = self.open_branch()
         except (errors.NotBranchError, errors.NoRepositoryPresent):
             pass
 
-        # colocated branches
-        ret.extend([self.open_branch(name.decode("utf-8")) for name in
-                    self._read_branch_list()])
+        for name in self._read_branch_list():
+            ret[name] = self.open_branch(name.decode('utf-8'))
 
         return ret
 
 
=== modified file 'bzrlib/controldir.py'
--- bzrlib/controldir.py	2011-12-07 14:49:51 +0000
+++ bzrlib/controldir.py	2011-12-11 04:20:30 +0000
@@ -106,10 +106,14 @@
         """Return a sequence of all branches local to this control directory.
 
         """
+        return self.get_branches().values()
+
+    def get_branches(self):
+        """Return a dictionary with branch_names and branch objects."""
         try:
-            return [self.open_branch()]
+           return {None:self.open_branch()}
         except (errors.NotBranchError, errors.NoRepositoryPresent):
-            return []
+           return {}
 
     def is_control_filename(self, filename):
         """True if filename is the name of a path which is reserved for
 
=== modified file 'bzrlib/tests/blackbox/test_branch.py'
--- bzrlib/tests/blackbox/test_branch.py	2011-11-17 19:50:38 +0000
+++ bzrlib/tests/blackbox/test_branch.py	2011-12-11 04:20:30 +0000
@@ -78,7 +78,7 @@
         self.assertEqual('', out)
         self.assertEqual('Branched 2 revisions.\n', err)
         out, err = self.run_bzr('branches b')
-        self.assertEqual(" orig\n thiswasa\n", out)
+        self.assertEqual(" thiswasa\n orig\n", out)
         self.assertEqual('', err)
         out,err = self.run_bzr('branch a file:b,branch=orig', retcode=3)
         self.assertEqual('', out)
 
=== modified file 'bzrlib/tests/per_bzrdir/test_bzrdir.py'
--- bzrlib/tests/per_bzrdir/test_bzrdir.py	2011-11-21 18:59:51 +0000
+++ bzrlib/tests/per_bzrdir/test_bzrdir.py	2011-12-11 04:20:30 +0000
@@ -21,6 +21,7 @@
 
 import bzrlib.branch
 from bzrlib import (
+    branch,
     bzrdir,
     errors,
     repository,
@@ -683,3 +684,14 @@
         self.assertEquals(
             branch.bzrdir.user_transport.list_dir("."),
             [".bzr"])
+
+    def test_get_branches(self):
+        repo = self.make_repository('branch-1')
+        try:
+            target_branch = repo.bzrdir.create_branch(name='foo')
+        except errors.NoColocatedBranchSupport:
+            return
+        reference = branch.BranchReferenceFormat().initialize(
+            repo.bzrdir, target_branch=target_branch)
+        self.assertEqual(set(repo.bzrdir.get_branches().keys()),
+                         set([None, 'foo']))
 
=== modified file 'bzrlib/tests/per_controldir/test_controldir.py'
--- bzrlib/tests/per_controldir/test_controldir.py	2011-11-17 10:59:40 +0000
+++ bzrlib/tests/per_controldir/test_controldir.py	2011-12-11 04:20:30 +0000
@@ -1192,6 +1192,11 @@
         else:
             self.assertEquals([], made_control.list_branches())
 
+    def test_get_branches(self):
+        repo = self.make_repository('branch-1')
+        target_branch = repo.bzrdir.create_branch()
+        self.assertEqual(repo.bzrdir.get_branches().keys(), [None])
+
     def test_create_repository(self):
         # a bzrdir can construct a repository for itself.
         if not self.bzrdir_format.is_initializable():
 
=== modified file 'bzrlib/tests/per_controldir_colo/test_supported.py'
--- bzrlib/tests/per_controldir_colo/test_supported.py	2011-11-17 18:39:26 +0000
+++ bzrlib/tests/per_controldir_colo/test_supported.py	2011-12-11 04:20:30 +0000
@@ -127,3 +127,11 @@
         made_branch = Branch.open(made_branch.user_url)
         self.assertEquals(u"col\xe9", made_branch.name)
         made_control.destroy_branch(u"col\xe9")
+
+    def test_get_branches(self):
+        repo = self.make_repository('branch-1')
+        target_branch = repo.bzrdir.create_branch(name='foo')
+        self.assertEqual(repo.bzrdir.get_branches().keys(),
+                         ['foo'])
+        self.assertEqual(repo.bzrdir.get_branches()['foo'].base,
+                         target_branch.base)
 
=== modified file 'bzrlib/tests/per_controldir_colo/test_unsupported.py'
--- bzrlib/tests/per_controldir_colo/test_unsupported.py	2011-09-23 12:32:16 +0000
+++ bzrlib/tests/per_controldir_colo/test_unsupported.py	2011-12-11 04:20:30 +0000
@@ -68,3 +68,9 @@
         made_control = self.make_bzrdir_with_repo()
         self.assertRaises(errors.NoColocatedBranchSupport,
             made_control.get_branch_reference, "colo")
+
+    def test_get_branches(self):
+        made_control = self.make_bzrdir_with_repo()
+        made_control.create_branch()
+        self.assertEqual(made_control.get_branches().keys(),
+                         [None])
 
=== modified file 'doc/en/release-notes/bzr-2.5.txt'
--- doc/en/release-notes/bzr-2.5.txt	2011-12-09 12:04:25 +0000
+++ doc/en/release-notes/bzr-2.5.txt	2011-12-11 04:20:30 +0000
@@ -240,6 +240,11 @@
 * The registry of merge types has been moved to ``merge`` from ``option`` but
   ``merge.get_merge_type_registry`` remains as an accessor. (Martin Packman)
 
+* ControlDir now has a get_branches method that returns a dictionary
+  whose keys are the names of the branches and whose values are the
+  branches themselves.  Branches without names use the key None.
+  (Neil Martinsen-Burrell)
+
 Testing
 *******