Merge lp:~jelmer/bzr/switch-possible-transports into lp:bzr
- switch-possible-transports
- Merge into bzr.dev
Status: | Superseded |
---|---|
Proposed branch: | lp:~jelmer/bzr/switch-possible-transports |
Merge into: | lp:bzr |
Prerequisite: | lp:~jelmer/bzr/count-transport-connections |
Diff against target: |
374 lines (+79/-43) 11 files modified
bzrlib/builtins.py (+32/-20) bzrlib/bzrdir.py (+8/-2) bzrlib/controldir.py (+2/-1) bzrlib/plugins/weave_fmt/bzrdir.py (+2/-2) bzrlib/plugins/weave_fmt/workingtree.py (+3/-2) bzrlib/switch.py (+13/-6) bzrlib/tests/blackbox/test_switch.py (+1/-1) bzrlib/tests/test_workingtree.py (+2/-2) bzrlib/workingtree_3.py (+1/-1) bzrlib/workingtree_4.py (+6/-4) doc/en/release-notes/bzr-2.5.txt (+9/-2) |
To merge this branch: | bzr merge lp:~jelmer/bzr/switch-possible-transports |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Vincent Ladeuil | Needs Information | ||
Martin Packman | Pending | ||
Review via email: mp+85742@code.launchpad.net |
This proposal supersedes a proposal from 2011-12-14.
This proposal has been superseded by a proposal from 2011-12-21.
Commit message
Description of the change
Use possible_transports in 'bzr switch'.
This means that switching a lightweight checkout from one branch on Launchpad
to another branch on Launchpad no longer requires five connections, but just one.
This greatly speeds up switching.
Add a possible_transports argument to BzrDir.
necessary, as it opens branches, so it needs to pass possible_transports along
(e.g. in case the local branch is a branch reference).
Switch could still be improved to actually do fewer open_branch() calls,
but there's a separate bug about that - bug #812295
Martin Packman (gz) wrote : Posted in a previous version of this proposal | # |
Jelmer Vernooij (jelmer) wrote : | # |
I've now updated it to test the number of connections. You can nicely see the HPSS call count down from 38 down to 24, and the number of connections from 7 to 1. :-)
Vincent Ladeuil (vila) wrote : | # |
... profit ;)
28 + possible_
75 + possible_
97 + possible_
Huh ? This shouldn't be required, you already passed possible_transports
which, being a container, should have received the new transport (or reused
an existing one).
May be you added them before adding possible_transport as a parameter where
needed ? Having to do both has a weird smell otherwise...
I won't block on that but would like to understand what I'm missing here... (I would approve with both hands otherwise ;).
Jelmer Vernooij (jelmer) wrote : | # |
Am 21/12/11 11:36, schrieb Vincent Ladeuil:
> Review: Needs Information
>
> ... profit ;)
>
> 28 + possible_
> 75 + possible_
> 97 + possible_
>
> Huh ? This shouldn't be required, you already passed possible_transports
> which, being a container, should have received the new transport (or reused
> an existing one).
possible_transports is read-only as far as I know. It's always the
callers that update it, as far as I can tell. None of our code updates
possible_transports when it is passed AFAIK.
> May be you added them before adding possible_transport as a parameter where
> needed ? Having to do both has a weird smell otherwise...
>
> I won't block on that but would like to understand what I'm missing here... (I would approve with both hands otherwise ;).
:-) Thanks
Cheers,
Jelmer
Jelmer Vernooij (jelmer) wrote : | # |
You were right, bzrlib.transport already takes care of updating possible_
I've updated the branch to not mess with possible_transports in bzrlib.builtins, and the tests still pass.
Preview Diff
1 | === modified file 'bzrlib/builtins.py' |
2 | --- bzrlib/builtins.py 2011-12-20 18:47:35 +0000 |
3 | +++ bzrlib/builtins.py 2011-12-21 15:02:35 +0000 |
4 | @@ -1343,6 +1343,7 @@ |
5 | if to_location is None: |
6 | to_location = urlutils.derive_to_location(from_location) |
7 | to_transport = transport.get_transport(to_location) |
8 | + possible_transports = [to_transport] |
9 | try: |
10 | to_transport.mkdir('.') |
11 | except errors.FileExists: |
12 | @@ -1371,15 +1372,15 @@ |
13 | try: |
14 | # preserve whatever source format we have. |
15 | to_dir = br_from.bzrdir.sprout(to_transport.base, revision_id, |
16 | - possible_transports=[to_transport], |
17 | - accelerator_tree=accelerator_tree, |
18 | - hardlink=hardlink, stacked=stacked, |
19 | - force_new_repo=standalone, |
20 | - create_tree_if_local=not no_tree, |
21 | - source_branch=br_from) |
22 | + possible_transports=possible_transports, |
23 | + accelerator_tree=accelerator_tree, |
24 | + hardlink=hardlink, stacked=stacked, |
25 | + force_new_repo=standalone, |
26 | + create_tree_if_local=not no_tree, |
27 | + source_branch=br_from) |
28 | + possible_transports.append(br_from.bzrdir.root_transport) |
29 | branch = to_dir.open_branch( |
30 | - possible_transports=[ |
31 | - br_from.bzrdir.root_transport, to_transport]) |
32 | + possible_transports=possible_transports) |
33 | except errors.NoSuchRevision: |
34 | to_transport.delete_tree('.') |
35 | msg = gettext("The branch {0} has no revision {1}.").format( |
36 | @@ -1399,13 +1400,15 @@ |
37 | note(ngettext('Branched %d revision.', 'Branched %d revisions.', branch.revno()) % branch.revno()) |
38 | if bind: |
39 | # Bind to the parent |
40 | - parent_branch = Branch.open(from_location) |
41 | + parent_branch = Branch.open(from_location, |
42 | + possible_transports=possible_transports) |
43 | branch.bind(parent_branch) |
44 | note(gettext('New branch bound to %s') % from_location) |
45 | if switch: |
46 | # Switch to the new branch |
47 | wt, _ = WorkingTree.open_containing('.') |
48 | - _mod_switch.switch(wt.bzrdir, branch) |
49 | + _mod_switch.switch(wt.bzrdir, branch, |
50 | + possible_transports=possible_transports) |
51 | note(gettext('Switched to branch: %s'), |
52 | urlutils.unescape_for_display(branch.base, 'utf-8')) |
53 | |
54 | @@ -6091,14 +6094,17 @@ |
55 | from bzrlib import switch |
56 | tree_location = directory |
57 | revision = _get_one_revision('switch', revision) |
58 | - control_dir = controldir.ControlDir.open_containing(tree_location)[0] |
59 | + possible_transports = [] |
60 | + control_dir = controldir.ControlDir.open_containing( |
61 | + tree_location, possible_transports=possible_transports)[0] |
62 | if to_location is None: |
63 | if revision is None: |
64 | raise errors.BzrCommandError(gettext('You must supply either a' |
65 | ' revision or a location')) |
66 | to_location = tree_location |
67 | try: |
68 | - branch = control_dir.open_branch() |
69 | + branch = control_dir.open_branch( |
70 | + possible_transports=possible_transports) |
71 | had_explicit_nick = branch.get_config().has_explicit_nickname() |
72 | except errors.NotBranchError: |
73 | branch = None |
74 | @@ -6115,7 +6121,7 @@ |
75 | # Perhaps the target control dir supports colocated branches? |
76 | try: |
77 | root = controldir.ControlDir.open(this_url, |
78 | - possible_transports=[control_dir.user_transport]) |
79 | + possible_transports=possible_transports) |
80 | except errors.NotBranchError: |
81 | colocated = False |
82 | else: |
83 | @@ -6127,25 +6133,31 @@ |
84 | to_location = urlutils.join( |
85 | this_url, '..', urlutils.escape(to_location)) |
86 | to_branch = branch.bzrdir.sprout(to_location, |
87 | - possible_transports=[branch.bzrdir.root_transport], |
88 | - source_branch=branch).open_branch() |
89 | + possible_transports=possible_transports, |
90 | + source_branch=branch).open_branch( |
91 | + possible_transports=possible_transports) |
92 | else: |
93 | # Perhaps it's a colocated branch? |
94 | try: |
95 | - to_branch = control_dir.open_branch(to_location) |
96 | + to_branch = control_dir.open_branch( |
97 | + to_location, possible_transports=possible_transports) |
98 | except (errors.NotBranchError, errors.NoColocatedBranchSupport): |
99 | try: |
100 | - to_branch = Branch.open(to_location) |
101 | + to_branch = Branch.open(to_location, |
102 | + possible_transports=possible_transports) |
103 | except errors.NotBranchError: |
104 | this_url = self._get_branch_location(control_dir) |
105 | to_branch = Branch.open( |
106 | urlutils.join( |
107 | - this_url, '..', urlutils.escape(to_location))) |
108 | + this_url, '..', urlutils.escape(to_location)), |
109 | + possible_transports=possible_transports) |
110 | if revision is not None: |
111 | revision = revision.as_revision_id(to_branch) |
112 | - switch.switch(control_dir, to_branch, force, revision_id=revision) |
113 | + switch.switch(control_dir, to_branch, force, revision_id=revision, |
114 | + possible_transports=possible_transports) |
115 | if had_explicit_nick: |
116 | - branch = control_dir.open_branch() #get the new branch! |
117 | + #get the new branch! |
118 | + branch = control_dir.open_branch(possible_transports=possible_transports) |
119 | branch.nick = to_branch.nick |
120 | note(gettext('Switched to branch: %s'), |
121 | urlutils.unescape_for_display(to_branch.base, 'utf-8')) |
122 | |
123 | === modified file 'bzrlib/bzrdir.py' |
124 | --- bzrlib/bzrdir.py 2011-12-19 13:23:58 +0000 |
125 | +++ bzrlib/bzrdir.py 2011-12-21 15:02:35 +0000 |
126 | @@ -974,13 +974,19 @@ |
127 | return format.open(self, _found=True) |
128 | |
129 | def open_workingtree(self, unsupported=False, |
130 | - recommend_upgrade=True): |
131 | + recommend_upgrade=True, possible_transports=None): |
132 | """See BzrDir.open_workingtree.""" |
133 | from bzrlib.workingtree import WorkingTreeFormatMetaDir |
134 | format = WorkingTreeFormatMetaDir.find_format(self) |
135 | format.check_support_status(unsupported, recommend_upgrade, |
136 | basedir=self.root_transport.base) |
137 | - return format.open(self, _found=True) |
138 | + if possible_transports is None: |
139 | + possible_transports = [] |
140 | + else: |
141 | + possible_transports = list(possible_transports) + [ |
142 | + self.root_transport] |
143 | + return format.open(self, possible_transports=possible_transports, |
144 | + _found=True) |
145 | |
146 | def _get_config(self): |
147 | return config.TransportConfig(self.transport, 'control.conf') |
148 | |
149 | === modified file 'bzrlib/controldir.py' |
150 | --- bzrlib/controldir.py 2011-12-19 13:23:58 +0000 |
151 | +++ bzrlib/controldir.py 2011-12-21 15:02:35 +0000 |
152 | @@ -263,7 +263,8 @@ |
153 | raise NotImplementedError(self.find_repository) |
154 | |
155 | def open_workingtree(self, _unsupported=False, |
156 | - recommend_upgrade=True, from_branch=None): |
157 | + recommend_upgrade=True, from_branch=None, |
158 | + possible_transports=None): |
159 | """Open the workingtree object at this ControlDir if one is present. |
160 | |
161 | :param recommend_upgrade: Optional keyword parameter, when True (the |
162 | |
163 | === modified file 'bzrlib/plugins/weave_fmt/bzrdir.py' |
164 | --- bzrlib/plugins/weave_fmt/bzrdir.py 2011-12-19 13:23:58 +0000 |
165 | +++ bzrlib/plugins/weave_fmt/bzrdir.py 2011-12-21 15:02:35 +0000 |
166 | @@ -966,14 +966,14 @@ |
167 | from bzrlib.plugins.weave_fmt.repository import RepositoryFormat5 |
168 | return RepositoryFormat5().open(self, _found=True) |
169 | |
170 | - def open_workingtree(self, _unsupported=False, |
171 | + def open_workingtree(self, possible_transports=None, _unsupported=False, |
172 | recommend_upgrade=True): |
173 | """See BzrDir.create_workingtree.""" |
174 | from bzrlib.plugins.weave_fmt.workingtree import WorkingTreeFormat2 |
175 | wt_format = WorkingTreeFormat2() |
176 | # we don't warn here about upgrades; that ought to be handled for the |
177 | # bzrdir as a whole |
178 | - return wt_format.open(self, _found=True) |
179 | + return wt_format.open(self, possible_transports, _found=True) |
180 | |
181 | |
182 | class BzrDir6(BzrDirPreSplitOut): |
183 | |
184 | === modified file 'bzrlib/plugins/weave_fmt/workingtree.py' |
185 | --- bzrlib/plugins/weave_fmt/workingtree.py 2011-12-19 13:23:58 +0000 |
186 | +++ bzrlib/plugins/weave_fmt/workingtree.py 2011-12-21 15:02:35 +0000 |
187 | @@ -120,7 +120,7 @@ |
188 | from bzrlib.plugins.weave_fmt.bzrdir import BzrDirFormat6 |
189 | self._matchingbzrdir = BzrDirFormat6() |
190 | |
191 | - def open(self, a_bzrdir, _found=False): |
192 | + def open(self, a_bzrdir, possible_transports=None, _found=False): |
193 | """Return the WorkingTree object for a_bzrdir |
194 | |
195 | _found is a private parameter, do not use it. It is used to indicate |
196 | @@ -131,11 +131,12 @@ |
197 | raise NotImplementedError |
198 | if not isinstance(a_bzrdir.transport, LocalTransport): |
199 | raise errors.NotLocalUrl(a_bzrdir.transport.base) |
200 | + branch = a_bzrdir.open_branch(possible_transports) |
201 | wt = WorkingTree2(a_bzrdir.root_transport.local_abspath('.'), |
202 | _internal=True, |
203 | _format=self, |
204 | _bzrdir=a_bzrdir, |
205 | - _control_files=a_bzrdir.open_branch().control_files) |
206 | + _control_files=branch.control_files) |
207 | return wt |
208 | |
209 | |
210 | |
211 | === modified file 'bzrlib/switch.py' |
212 | --- bzrlib/switch.py 2011-12-18 15:28:38 +0000 |
213 | +++ bzrlib/switch.py 2011-12-21 15:02:35 +0000 |
214 | @@ -32,7 +32,9 @@ |
215 | for hook in hooks: |
216 | hook(params) |
217 | |
218 | -def switch(control_dir, to_branch, force=False, quiet=False, revision_id=None): |
219 | + |
220 | +def switch(control_dir, to_branch, force=False, quiet=False, revision_id=None, |
221 | + possible_transports=None): |
222 | """Switch the branch associated with a checkout. |
223 | |
224 | :param control_dir: ControlDir of the checkout to change |
225 | @@ -40,23 +42,28 @@ |
226 | :param force: skip the check for local commits in a heavy checkout |
227 | :param revision_id: revision ID to switch to. |
228 | """ |
229 | - _check_pending_merges(control_dir, force) |
230 | + _check_pending_merges(control_dir, possible_transports, force) |
231 | try: |
232 | - source_repository = control_dir.open_branch().repository |
233 | + from_branch = control_dir.open_branch( |
234 | + possible_transports=possible_transports) |
235 | + source_repository = from_branch.repository |
236 | except errors.NotBranchError: |
237 | source_repository = to_branch.repository |
238 | _set_branch_location(control_dir, to_branch, force) |
239 | - tree = control_dir.open_workingtree() |
240 | + tree = control_dir.open_workingtree( |
241 | + possible_transports=possible_transports) |
242 | _update(tree, source_repository, quiet, revision_id) |
243 | _run_post_switch_hooks(control_dir, to_branch, force, revision_id) |
244 | |
245 | -def _check_pending_merges(control, force=False): |
246 | + |
247 | +def _check_pending_merges(control, possible_transports=None, force=False): |
248 | """Check that there are no outstanding pending merges before switching. |
249 | |
250 | :param control: ControlDir of the branch to check |
251 | """ |
252 | try: |
253 | - tree = control.open_workingtree() |
254 | + tree = control.open_workingtree( |
255 | + possible_transports=possible_transports) |
256 | except errors.NotBranchError, ex: |
257 | # Lightweight checkout and branch is no longer there |
258 | if force: |
259 | |
260 | === modified file 'bzrlib/tests/blackbox/test_switch.py' |
261 | --- bzrlib/tests/blackbox/test_switch.py 2011-12-21 15:02:33 +0000 |
262 | +++ bzrlib/tests/blackbox/test_switch.py 2011-12-21 15:02:35 +0000 |
263 | @@ -449,5 +449,5 @@ |
264 | # become necessary for this use case. Please do not adjust this number |
265 | # upwards without agreement from bzr's network support maintainers. |
266 | self.assertLength(24, self.hpss_calls) |
267 | - self.assertLength(5, self.hpss_connections) |
268 | + self.assertLength(1, self.hpss_connections) |
269 | self.assertThat(self.hpss_calls, ContainsNoVfsCalls) |
270 | |
271 | === modified file 'bzrlib/tests/test_workingtree.py' |
272 | --- bzrlib/tests/test_workingtree.py 2011-12-11 02:43:03 +0000 |
273 | +++ bzrlib/tests/test_workingtree.py 2011-12-21 15:02:35 +0000 |
274 | @@ -149,7 +149,7 @@ |
275 | def is_supported(self): |
276 | return False |
277 | |
278 | - def open(self, transport, _found=False): |
279 | + def open(self, path, possible_transports=None, _found=False): |
280 | return "opened tree." |
281 | |
282 | |
283 | @@ -169,7 +169,7 @@ |
284 | def is_supported(self): |
285 | return False |
286 | |
287 | - def open(self, transport, _found=False): |
288 | + def open(self, path, possible_transports=None, _found=False): |
289 | raise NotImplementedError(self.open) |
290 | |
291 | |
292 | |
293 | === modified file 'bzrlib/workingtree_3.py' |
294 | --- bzrlib/workingtree_3.py 2011-12-19 13:23:58 +0000 |
295 | +++ bzrlib/workingtree_3.py 2011-12-21 15:02:35 +0000 |
296 | @@ -237,7 +237,7 @@ |
297 | def _initial_inventory(self): |
298 | return inventory.Inventory() |
299 | |
300 | - def open(self, a_bzrdir, _found=False): |
301 | + def open(self, a_bzrdir, possible_transports=None, _found=False): |
302 | """Return the WorkingTree object for a_bzrdir |
303 | |
304 | _found is a private parameter, do not use it. It is used to indicate |
305 | |
306 | === modified file 'bzrlib/workingtree_4.py' |
307 | --- bzrlib/workingtree_4.py 2011-12-19 17:39:35 +0000 |
308 | +++ bzrlib/workingtree_4.py 2011-12-21 15:02:35 +0000 |
309 | @@ -1560,7 +1560,7 @@ |
310 | :param wt: the WorkingTree object |
311 | """ |
312 | |
313 | - def open(self, a_bzrdir, _found=False): |
314 | + def open(self, a_bzrdir, possible_transports=None, _found=False): |
315 | """Return the WorkingTree object for a_bzrdir |
316 | |
317 | _found is a private parameter, do not use it. It is used to indicate |
318 | @@ -1571,17 +1571,19 @@ |
319 | raise NotImplementedError |
320 | if not isinstance(a_bzrdir.transport, LocalTransport): |
321 | raise errors.NotLocalUrl(a_bzrdir.transport.base) |
322 | - wt = self._open(a_bzrdir, self._open_control_files(a_bzrdir)) |
323 | + wt = self._open(a_bzrdir, self._open_control_files(a_bzrdir), |
324 | + possible_transports) |
325 | return wt |
326 | |
327 | - def _open(self, a_bzrdir, control_files): |
328 | + def _open(self, a_bzrdir, control_files, possible_transports=None): |
329 | """Open the tree itself. |
330 | |
331 | :param a_bzrdir: the dir for the tree. |
332 | :param control_files: the control files for the tree. |
333 | """ |
334 | return self._tree_class(a_bzrdir.root_transport.local_abspath('.'), |
335 | - branch=a_bzrdir.open_branch(), |
336 | + branch=a_bzrdir.open_branch( |
337 | + possible_transports=possible_transports), |
338 | _format=self, |
339 | _bzrdir=a_bzrdir, |
340 | _control_files=control_files) |
341 | |
342 | === modified file 'doc/en/release-notes/bzr-2.5.txt' |
343 | --- doc/en/release-notes/bzr-2.5.txt 2011-12-21 00:07:49 +0000 |
344 | +++ doc/en/release-notes/bzr-2.5.txt 2011-12-21 15:02:35 +0000 |
345 | @@ -36,13 +36,17 @@ |
346 | |
347 | * New HPSS call for ``Repository.reconcile``. (Jelmer Vernooij, #894455) |
348 | |
349 | +* ``bzr send`` now only opens a single connection, rather than two, |
350 | + to the target branch. (Jelmer Vernooij) |
351 | + |
352 | * Override the value returned by ``sys.getfilesystemencoding()`` for the bzr |
353 | script to utf-8 when it would otherwise be ascii on a posix system. This |
354 | will mean bzr works with non-ascii files when no locale or an incorrect |
355 | locale is set. (Martin Packman, #794353) |
356 | |
357 | -* ``bzr send`` now only opens a single connection, rather than two, |
358 | - to the target branch. (Jelmer Vernooij) |
359 | +* Switching a lightweight checkout to a new branch opens only a single |
360 | + external connection now per branch rather than 5. |
361 | + (Jelmer Vernooij) |
362 | |
363 | Bug Fixes |
364 | ********* |
365 | @@ -87,6 +91,9 @@ |
366 | .. Changes that may require updates in plugins or other code that uses |
367 | bzrlib. |
368 | |
369 | +* ``BzrDir.open_workingtree`` and ``WorkingTreeFormat.open`` now take an optional |
370 | + ``possible_transports`` argument. (Jelmer Vernooij) |
371 | + |
372 | * ``Config.signature_needed``, ``Config.signing_policy``, |
373 | ``Config.gpg_signing_key``, ``Config.gpg_signing_command``, |
374 | ``Config.checking_policy`` and ``Config.post_commit`` are now deprecated. |
This seems like a pretty reasonable interface change overall. Though not reopening the same branch should be the end goal, you'd still want this so that opening different remote branches over the same transport would work.
Does seem like some automated testing is needed. Even if we don't have infrastructure for tracking connection opens at the moment for the switch side, checking that open_workingtree passes the transport down correctly seems necessary.