Merge lp:~nmb/bzr/893470-dedupe-list-branches into lp:bzr
- 893470-dedupe-list-branches
- Merge into bzr.dev
Status: | Merged |
---|---|
Approved by: | Vincent Ladeuil |
Approved revision: | no longer in the source branch. |
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 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Vincent Ladeuil | Needs Fixing | ||
Jelmer Vernooij (community) | Approve | ||
Review via email: mp+83075@code.launchpad.net |
Commit message
Description of the change
This checks to see if the current default branch is one of the colocated branches before returning it in BzrDir.
Jelmer Vernooij (jelmer) wrote : | # |
Jelmer Vernooij (jelmer) : | # |
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[
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[
> 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.
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.
"trunk": Branch(
Branch(
Branch("http://
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_
# Create a branch branch-1 that initially is a checkout of 'foo'
# Use switch to create 'anotherbranch' which derives from that
repo = self.make_
tree = repo.bzrdir.
repo.bzrdir.
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_
> # Create a branch branch-1 that initially is a checkout of 'foo'
> # Use switch to create 'anotherbranch' which derives from that
> repo = self.make_
> target_branch = repo.bzrdir.
> branch.
> repo.bzrdir, target_
> tree = repo.bzrdir.
>
> repo.bzrdir.
{'foo': Branch(
Branch(
IOW, I think we should just open the branch as .open_branch() would.
Cheers,
Jelmer
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.
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_
76 + repo = self.make_
77 + target_branch = repo.bzrdir.
78 + reference = branch.
79 + repo.bzrdir, target_
^^ These tests are not bzr-specific, they can be run against e.g. svn as well, where BranchReference
Cheers,
Jelmer
Jelmer Vernooij (jelmer) wrote : | # |
We also need a test in bzrlib/
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_
Neil Martinsen-Burrell (nmb) wrote : | # |
I put the BranchReference test in per_bzrdir/
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) : | # |
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.assertEqua
89 + set([None, 'foo']))
101 + self.assertEqua
118 + self.assertEqua
119 + ['foo'])
120 + self.assertEqua
121 + target_branch.base)
134 + self.assertEqua
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/
Both of this tweaks can be done while landing.
Preview Diff
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 |
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.