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

Proposed by Neil Martinsen-Burrell on 2011-11-22
Status: Merged
Approved by: Vincent Ladeuil on 2011-12-12
Approved revision: 6292
Merged at revision: 6356
Proposed branch: lp:~nmb/bzr/893470-dedupe-list-branches
Merge into: lp:bzr
Diff against target: 150 lines (+49/-11)
8 files modified
bzrlib/bzrdir.py (+6/-8)
bzrlib/controldir.py (+6/-2)
bzrlib/tests/blackbox/test_branch.py (+1/-1)
bzrlib/tests/per_bzrdir/test_bzrdir.py (+12/-0)
bzrlib/tests/per_controldir/test_controldir.py (+5/-0)
bzrlib/tests/per_controldir_colo/test_supported.py (+8/-0)
bzrlib/tests/per_controldir_colo/test_unsupported.py (+6/-0)
doc/en/release-notes/bzr-2.5.txt (+5/-0)
To merge this branch: bzr merge lp:~nmb/bzr/893470-dedupe-list-branches
Reviewer Review Type Date Requested Status
Vincent Ladeuil Needs Fixing on 2011-12-12
Jelmer Vernooij (community) 2011-11-22 Approve on 2011-12-11
Review via email: mp+83075@code.launchpad.net

Description of the change

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().

To post a comment you must log in.
Jelmer Vernooij (jelmer) wrote :

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) wrote :

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) wrote :

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) wrote :

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) wrote :

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) wrote :

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

6287. By Neil Martinsen-Burrell on 2011-12-09

add news entry

6288. By Neil Martinsen-Burrell on 2011-12-09

fix test broken by old approach to fixing bug

6289. By Neil Martinsen-Burrell on 2011-12-09

undo left-over change

Neil Martinsen-Burrell (nmb) wrote :

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) wrote :

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) wrote :

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) wrote :

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().

6290. By Neil Martinsen-Burrell on 2011-12-11

refine tests

6291. By Neil Martinsen-Burrell on 2011-12-11

implement list_branches in terms of get_branches

6292. By Neil Martinsen-Burrell on 2011-12-11

fix test fallout

Neil Martinsen-Burrell (nmb) wrote :

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) wrote :

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) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/bzrdir.py'
2--- bzrlib/bzrdir.py 2011-12-11 03:44:06 +0000
3+++ bzrlib/bzrdir.py 2011-12-11 04:20:30 +0000
4@@ -1054,18 +1054,16 @@
5 self.control_files.unlock()
6 self.transport.delete_tree(path)
7
8- def list_branches(self):
9- """See ControlDir.list_branches."""
10- ret = []
11- # Default branch
12+ def get_branches(self):
13+ """See ControlDir.get_branches."""
14+ ret = {}
15 try:
16- ret.append(self.open_branch())
17+ ret[None] = self.open_branch()
18 except (errors.NotBranchError, errors.NoRepositoryPresent):
19 pass
20
21- # colocated branches
22- ret.extend([self.open_branch(name.decode("utf-8")) for name in
23- self._read_branch_list()])
24+ for name in self._read_branch_list():
25+ ret[name] = self.open_branch(name.decode('utf-8'))
26
27 return ret
28
29
30=== modified file 'bzrlib/controldir.py'
31--- bzrlib/controldir.py 2011-12-07 14:49:51 +0000
32+++ bzrlib/controldir.py 2011-12-11 04:20:30 +0000
33@@ -106,10 +106,14 @@
34 """Return a sequence of all branches local to this control directory.
35
36 """
37+ return self.get_branches().values()
38+
39+ def get_branches(self):
40+ """Return a dictionary with branch_names and branch objects."""
41 try:
42- return [self.open_branch()]
43+ return {None:self.open_branch()}
44 except (errors.NotBranchError, errors.NoRepositoryPresent):
45- return []
46+ return {}
47
48 def is_control_filename(self, filename):
49 """True if filename is the name of a path which is reserved for
50
51=== modified file 'bzrlib/tests/blackbox/test_branch.py'
52--- bzrlib/tests/blackbox/test_branch.py 2011-11-17 19:50:38 +0000
53+++ bzrlib/tests/blackbox/test_branch.py 2011-12-11 04:20:30 +0000
54@@ -78,7 +78,7 @@
55 self.assertEqual('', out)
56 self.assertEqual('Branched 2 revisions.\n', err)
57 out, err = self.run_bzr('branches b')
58- self.assertEqual(" orig\n thiswasa\n", out)
59+ self.assertEqual(" thiswasa\n orig\n", out)
60 self.assertEqual('', err)
61 out,err = self.run_bzr('branch a file:b,branch=orig', retcode=3)
62 self.assertEqual('', out)
63
64=== modified file 'bzrlib/tests/per_bzrdir/test_bzrdir.py'
65--- bzrlib/tests/per_bzrdir/test_bzrdir.py 2011-11-21 18:59:51 +0000
66+++ bzrlib/tests/per_bzrdir/test_bzrdir.py 2011-12-11 04:20:30 +0000
67@@ -21,6 +21,7 @@
68
69 import bzrlib.branch
70 from bzrlib import (
71+ branch,
72 bzrdir,
73 errors,
74 repository,
75@@ -683,3 +684,14 @@
76 self.assertEquals(
77 branch.bzrdir.user_transport.list_dir("."),
78 [".bzr"])
79+
80+ def test_get_branches(self):
81+ repo = self.make_repository('branch-1')
82+ try:
83+ target_branch = repo.bzrdir.create_branch(name='foo')
84+ except errors.NoColocatedBranchSupport:
85+ return
86+ reference = branch.BranchReferenceFormat().initialize(
87+ repo.bzrdir, target_branch=target_branch)
88+ self.assertEqual(set(repo.bzrdir.get_branches().keys()),
89+ set([None, 'foo']))
90
91=== modified file 'bzrlib/tests/per_controldir/test_controldir.py'
92--- bzrlib/tests/per_controldir/test_controldir.py 2011-11-17 10:59:40 +0000
93+++ bzrlib/tests/per_controldir/test_controldir.py 2011-12-11 04:20:30 +0000
94@@ -1192,6 +1192,11 @@
95 else:
96 self.assertEquals([], made_control.list_branches())
97
98+ def test_get_branches(self):
99+ repo = self.make_repository('branch-1')
100+ target_branch = repo.bzrdir.create_branch()
101+ self.assertEqual(repo.bzrdir.get_branches().keys(), [None])
102+
103 def test_create_repository(self):
104 # a bzrdir can construct a repository for itself.
105 if not self.bzrdir_format.is_initializable():
106
107=== modified file 'bzrlib/tests/per_controldir_colo/test_supported.py'
108--- bzrlib/tests/per_controldir_colo/test_supported.py 2011-11-17 18:39:26 +0000
109+++ bzrlib/tests/per_controldir_colo/test_supported.py 2011-12-11 04:20:30 +0000
110@@ -127,3 +127,11 @@
111 made_branch = Branch.open(made_branch.user_url)
112 self.assertEquals(u"col\xe9", made_branch.name)
113 made_control.destroy_branch(u"col\xe9")
114+
115+ def test_get_branches(self):
116+ repo = self.make_repository('branch-1')
117+ target_branch = repo.bzrdir.create_branch(name='foo')
118+ self.assertEqual(repo.bzrdir.get_branches().keys(),
119+ ['foo'])
120+ self.assertEqual(repo.bzrdir.get_branches()['foo'].base,
121+ target_branch.base)
122
123=== modified file 'bzrlib/tests/per_controldir_colo/test_unsupported.py'
124--- bzrlib/tests/per_controldir_colo/test_unsupported.py 2011-09-23 12:32:16 +0000
125+++ bzrlib/tests/per_controldir_colo/test_unsupported.py 2011-12-11 04:20:30 +0000
126@@ -68,3 +68,9 @@
127 made_control = self.make_bzrdir_with_repo()
128 self.assertRaises(errors.NoColocatedBranchSupport,
129 made_control.get_branch_reference, "colo")
130+
131+ def test_get_branches(self):
132+ made_control = self.make_bzrdir_with_repo()
133+ made_control.create_branch()
134+ self.assertEqual(made_control.get_branches().keys(),
135+ [None])
136
137=== modified file 'doc/en/release-notes/bzr-2.5.txt'
138--- doc/en/release-notes/bzr-2.5.txt 2011-12-09 12:04:25 +0000
139+++ doc/en/release-notes/bzr-2.5.txt 2011-12-11 04:20:30 +0000
140@@ -240,6 +240,11 @@
141 * The registry of merge types has been moved to ``merge`` from ``option`` but
142 ``merge.get_merge_type_registry`` remains as an accessor. (Martin Packman)
143
144+* ControlDir now has a get_branches method that returns a dictionary
145+ whose keys are the names of the branches and whose values are the
146+ branches themselves. Branches without names use the key None.
147+ (Neil Martinsen-Burrell)
148+
149 Testing
150 *******
151