Merge lp:~jelmer/bzr/rmbranch-colo into lp:bzr

Proposed by Jelmer Vernooij
Status: Merged
Approved by: Martin Packman
Approved revision: 6480
Merged at revision: 6481
Proposed branch: lp:~jelmer/bzr/rmbranch-colo
Merge into: lp:bzr
Diff against target: 149 lines (+53/-17)
4 files modified
bzrlib/builtins.py (+37/-14)
bzrlib/tests/blackbox/test_rmbranch.py (+12/-2)
bzrlib/tests/blackbox/test_switch.py (+1/-1)
doc/en/release-notes/bzr-2.6.txt (+3/-0)
To merge this branch: bzr merge lp:~jelmer/bzr/rmbranch-colo
Reviewer Review Type Date Requested Status
Martin Packman (community) Approve
Review via email: mp+94931@code.launchpad.net

This proposal supersedes a proposal from 2012-02-24.

Commit message

Support removing colocated branches in 'bzr rmbranch'.

Description of the change

Support removing colocated branches in 'bzr rmbranch'.

Also includes a drive-by fix to reduce the number of HPSS connections for 'bzr switch' in lightweight checkouts by one.

To post a comment you must log in.
Revision history for this message
Martin Packman (gz) wrote : Posted in a previous version of this proposal

Substative changes all seems fine.

Is this intended to go on 2.5 or dev? Either needs retargetting or release notes need moving (and conflict to resolve there anyway).

-def lookup_sibling_branch(control_dir, location, possible_transports=None):
- """Lookup sibling branch.
-
+def open_sibling_branch(control_dir, location, possible_transports=None):
+ """Open a branch, possibly a sibling.

This looks technically like an api change if it's what's in 2.5.0 but as it's pretty new I guess it's okay to do.

- self.assertLength(2, self.hpss_calls)
+ self.assertLength(5, self.hpss_calls)

Sneaking this up is from looking at more possible branch locations?

review: Needs Information
Revision history for this message
Jelmer Vernooij (jelmer) wrote : Posted in a previous version of this proposal

On 02/26/2012 11:17 PM, Martin Packman wrote:
> Review: Needs Information
>
> Substative changes all seems fine.
>
> Is this intended to go on 2.5 or dev? Either needs retargetting or release notes need moving (and conflict to resolve there anyway).
Yeah, mostly because it's a bug that people can remove active colocated
branches.

Alternatively, I could propose it against bzr.dev. colo support in 2.5
isn't fully polished anyway. That would mean fewer (risky) changes in
2.5. What do you think?
>
> -def lookup_sibling_branch(control_dir, location, possible_transports=None):
> - """Lookup sibling branch.
> -
> +def open_sibling_branch(control_dir, location, possible_transports=None):
> + """Open a branch, possibly a sibling.
>
> This looks technically like an api change if it's what's in 2.5.0 but as it's pretty new I guess it's okay to do.
I think that should be fine considering it's a fairly recent change,
indeed - and it's something that can't really be something that's used
by outsiders yet.
>
> - self.assertLength(2, self.hpss_calls)
> + self.assertLength(5, self.hpss_calls)
>
> Sneaking this up is from looking at more possible branch locations?
Yeah.

Cheers,

Jelmer

Revision history for this message
Jelmer Vernooij (jelmer) wrote : Posted in a previous version of this proposal

Now updated to target 2.6.

Revision history for this message
Martin Packman (gz) wrote :

Thanks for the extra details. This change does not seem unreasonable for 2.5, and the dependant branch probably does want fixing there. It can always be backported though, so this should probably just land as is.

review: Approve
Revision history for this message
Jelmer Vernooij (jelmer) wrote :

sent to pqm by email

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/builtins.py'
2--- bzrlib/builtins.py 2012-02-20 12:40:02 +0000
3+++ bzrlib/builtins.py 2012-02-28 10:07:20 +0000
4@@ -149,9 +149,9 @@
5 return location
6
7
8-def lookup_sibling_branch(control_dir, location, possible_transports=None):
9- """Lookup sibling branch.
10-
11+def open_sibling_branch(control_dir, location, possible_transports=None):
12+ """Open a branch, possibly a sibling.
13+
14 :param control_dir: Control directory relative to which to lookup the
15 location.
16 :param location: Location to look up
17@@ -162,13 +162,31 @@
18 return control_dir.open_branch(location,
19 possible_transports=possible_transports)
20 except (errors.NotBranchError, errors.NoColocatedBranchSupport):
21+ this_url = _get_branch_location(control_dir)
22+ return Branch.open(
23+ urlutils.join(
24+ this_url, '..', urlutils.escape(location)))
25+
26+
27+def open_nearby_branch(near=None, location=None, possible_transports=None):
28+ """Open a nearby branch.
29+
30+ :param near: Optional location of container from which to open branch
31+ :param location: Location of the branch
32+ :return: Branch instance
33+ """
34+ if near is None:
35+ if location is None:
36+ location = "."
37 try:
38- return Branch.open(location)
39+ return Branch.open(location,
40+ possible_transports=possible_transports)
41 except errors.NotBranchError:
42- this_url = _get_branch_location(control_dir)
43- return Branch.open(
44- urlutils.join(
45- this_url, '..', urlutils.escape(location)))
46+ near = "."
47+ cdir = controldir.ControlDir.open(near,
48+ possible_transports=possible_transports)
49+ return open_sibling_branch(cdir, location,
50+ possible_transports=possible_transports)
51
52
53 @symbol_versioning.deprecated_function(symbol_versioning.deprecated_in((2, 3, 0)))
54@@ -6251,7 +6269,12 @@
55 possible_transports=possible_transports,
56 source_branch=branch).open_branch()
57 else:
58- to_branch = lookup_sibling_branch(control_dir, to_location)
59+ try:
60+ to_branch = Branch.open(to_location,
61+ possible_transports=possible_transports)
62+ except errors.NotBranchError:
63+ to_branch = open_sibling_branch(control_dir, to_location,
64+ possible_transports=possible_transports)
65 if revision is not None:
66 revision = revision.as_revision_id(to_branch)
67 switch.switch(control_dir, to_branch, force, revision_id=revision)
68@@ -6454,13 +6477,13 @@
69
70 takes_args = ["location?"]
71
72+ takes_options = ['directory']
73+
74 aliases = ["rmbranch"]
75
76- def run(self, location=None):
77- if location is None:
78- location = "."
79- cdir = controldir.ControlDir.open_containing(location)[0]
80- cdir.destroy_branch()
81+ def run(self, directory=None, location=None):
82+ br = open_nearby_branch(near=directory, location=location)
83+ br.bzrdir.destroy_branch(br.name)
84
85
86 class cmd_shelve(Command):
87
88=== modified file 'bzrlib/tests/blackbox/test_rmbranch.py'
89--- bzrlib/tests/blackbox/test_rmbranch.py 2012-01-25 17:31:04 +0000
90+++ bzrlib/tests/blackbox/test_rmbranch.py 2012-02-28 10:07:20 +0000
91@@ -62,7 +62,7 @@
92
93 def test_remove_colo(self):
94 # Remove a colocated branch.
95- tree = self.example_branch('a', format='development-colo')
96+ tree = self.example_branch('a')
97 tree.bzrdir.create_branch(name="otherbranch")
98 self.assertTrue(tree.bzrdir.has_branch('otherbranch'))
99 self.run_bzr('rmbranch %s,branch=otherbranch' % tree.bzrdir.user_url)
100@@ -70,6 +70,16 @@
101 self.assertFalse(dir.has_branch('otherbranch'))
102 self.assertTrue(dir.has_branch())
103
104+ def test_remove_colo_directory(self):
105+ # Remove a colocated branch.
106+ tree = self.example_branch('a')
107+ tree.bzrdir.create_branch(name="otherbranch")
108+ self.assertTrue(tree.bzrdir.has_branch('otherbranch'))
109+ self.run_bzr('rmbranch otherbranch -d %s' % tree.bzrdir.user_url)
110+ dir = bzrdir.BzrDir.open('a')
111+ self.assertFalse(dir.has_branch('otherbranch'))
112+ self.assertTrue(dir.has_branch())
113+
114
115 class TestSmartServerRemoveBranch(TestCaseWithTransport):
116
117@@ -83,6 +93,6 @@
118 # being too low. If rpc_count increases, more network roundtrips have
119 # become necessary for this use case. Please do not adjust this number
120 # upwards without agreement from bzr's network support maintainers.
121- self.assertLength(2, self.hpss_calls)
122+ self.assertLength(5, self.hpss_calls)
123 self.assertLength(1, self.hpss_connections)
124 self.assertThat(self.hpss_calls, ContainsNoVfsCalls)
125
126=== modified file 'bzrlib/tests/blackbox/test_switch.py'
127--- bzrlib/tests/blackbox/test_switch.py 2012-02-18 16:55:04 +0000
128+++ bzrlib/tests/blackbox/test_switch.py 2012-02-28 10:07:20 +0000
129@@ -475,5 +475,5 @@
130 # become necessary for this use case. Please do not adjust this number
131 # upwards without agreement from bzr's network support maintainers.
132 self.assertLength(24, self.hpss_calls)
133- self.assertLength(5, self.hpss_connections)
134+ self.assertLength(4, self.hpss_connections)
135 self.assertThat(self.hpss_calls, ContainsNoVfsCalls)
136
137=== modified file 'doc/en/release-notes/bzr-2.6.txt'
138--- doc/en/release-notes/bzr-2.6.txt 2012-02-25 14:13:19 +0000
139+++ doc/en/release-notes/bzr-2.6.txt 2012-02-28 10:07:20 +0000
140@@ -34,6 +34,9 @@
141 * Avoid 'Invalid range access' errors when whole files are retrieved with
142 transport.http.get() . (Vincent Ladeuil, #924746)
143
144+* ``bzr rmbranch`` now supports removing colocated branches.
145+ (Jelmer Vernooij, #920653)
146+
147 * Two new command hooks, ``pre_command`` and ``post_command``,
148 provide notification before and after a command has been run.
149 (Brian de Alwis, Jelmer Vernooij)