Bazaar

Merge lp:~spiv/bzr/stacking-friendly-revision-history-verb into lp:~bzr/bzr/trunk-old

  1. stacking-friendly-revision-history-verb
  2. Merge into trunk-old
Proposed by Andrew Bennetts
Status: Merged
Merge reported by: Martin Pool
Merged at revision: not available
Proposed branch: lp:~spiv/bzr/stacking-friendly-revision-history-verb
Merge into: lp:~bzr/bzr/trunk-old
Diff against target: 492 lines
To merge this branch: bzr merge lp:~spiv/bzr/stacking-friendly-revision-history-verb
Reviewer Review Type Date Requested Status
Martin Pool Approve
Review via email: mp+7314@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Andrew Bennetts (spiv) wrote :

This fixes bug 380314. It is a cheap fix that simply falls back to VFS for
RemoteBranch._gen_revision_history if the branch is stacked. I've added tests
for both the blackbox case of "'bzr pull -r 123' from stacked" and a per-branch
test for .revision_history when there is stacking.

I have a followup patch in the works that defines a new RPC that avoids VFS
entirely in this case, but it's close to the release and getting the correctness
fix merged is more urgent.

Revision history for this message
Andrew Bennetts (spiv) wrote :

To clarify: this proposal is only for r4423 of my branch. The diff shown inline on Launchpad has this content. The longer (>400 line diff) you can download via the librarian includes in the (not quite ready) followup work.

Revision history for this message
Martin Pool (mbp) wrote :

Does this need a NEWS entry?

Please put the bug url in a comment in the tests to give more context.

The rest of these are general comments are needn't delay merging:

65 + # This figure represent the amount of work to perform this use case. It
66 + # is entirely ok to reduce this number if a test fails due to rpc_count
67 + # being too low. If rpc_count increases, more network roundtrips have
68 + # become necessary for this use case. Please do not adjust this number
69 + # upwards without agreement from bzr's network support maintainers.
70 + self.assertLength(43, self.hpss_calls)

This feels like it should be a specific assertion rather than copy and pasted so many times...

It's unfortunate to hardcode 1.9 in these tests but we should probably scan for them later.

88 + except unstackable_format_errors, e:
89 + raise TestNotApplicable('Format does not support stacking.')

Some people would say you should check at the start of the test whether it promises to support stacking.

Looks good.

review: Approve
Revision history for this message
Andrew Bennetts (spiv) wrote :

> Does this need a NEWS entry?

Yes, I'll add one.

> Please put the bug url in a comment in the tests to give more context.

Ok.

> The rest of these are general comments are needn't delay merging:
>
> 65 + # This figure represent the amount of work to perform this use case.
> It
> 66 + # is entirely ok to reduce this number if a test fails due to
> rpc_count
> 67 + # being too low. If rpc_count increases, more network roundtrips
> have
> 68 + # become necessary for this use case. Please do not adjust this
> number
> 69 + # upwards without agreement from bzr's network support maintainers.
> 70 + self.assertLength(43, self.hpss_calls)
>
> This feels like it should be a specific assertion rather than copy and pasted
> so many times...

Yeah. Another possibility is a centralised list of ratchet values somewhere, and then the scary "DO NOT CHANGE UPWARDS" comment can be at that one place.

> It's unfortunate to hardcode 1.9 in these tests but we should probably scan
> for them later.

Yeah. That's how Robert and I feel about the hardcoded 1.9 in these tests too.

> 88 + except unstackable_format_errors, e:
> 89 + raise TestNotApplicable('Format does not support stacking.')
>
> Some people would say you should check at the start of the test whether it
> promises to support stacking.

Well, I'm doing what nearby tests do. (Also, you can't do it at the very start, because at least for the Remote* tests you can't ask the format if it supports stacking unless there is an underlying object for the Remote*Format to ask).

> Looks good.

Thanks!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/branch.py'
2--- bzrlib/branch.py 2009-06-10 03:56:49 +0000
3+++ bzrlib/branch.py 2009-06-11 19:35:21 +0000
4@@ -91,6 +91,7 @@
5 self._revision_history_cache = None
6 self._revision_id_to_revno_cache = None
7 self._partial_revision_id_to_revno_cache = {}
8+ self._partial_revision_history_cache = []
9 self._last_revision_info_cache = None
10 self._merge_sorted_revisions_cache = None
11 self._open_hook()
12@@ -125,6 +126,27 @@
13 raise errors.UnstackableRepositoryFormat(self.repository._format,
14 self.repository.base)
15
16+ def _extend_partial_history(self, stop_index=None, stop_revision=None):
17+ """Extend the partial history to include a given index
18+
19+ If a stop_index is supplied, stop when that index has been reached.
20+ If a stop_revision is supplied, stop when that revision is
21+ encountered. Otherwise, stop when the beginning of history is
22+ reached.
23+
24+ :param stop_index: The index which should be present. When it is
25+ present, history extension will stop.
26+ :param stop_revision: The revision id which should be present. When
27+ it is encountered, history extension will stop.
28+ """
29+ if len(self._partial_revision_history_cache) == 0:
30+ self._partial_revision_history_cache = [self.last_revision()]
31+ repository._iter_for_revno(
32+ self.repository, self._partial_revision_history_cache,
33+ stop_index=stop_index, stop_revision=stop_revision)
34+ if self._partial_revision_history_cache[-1] == _mod_revision.NULL_REVISION:
35+ self._partial_revision_history_cache.pop()
36+
37 @staticmethod
38 def open(base, _unsupported=False, possible_transports=None):
39 """Open the branch rooted at base.
40@@ -698,6 +720,8 @@
41 self._revision_id_to_revno_cache = None
42 self._last_revision_info_cache = None
43 self._merge_sorted_revisions_cache = None
44+ self._partial_revision_history_cache = []
45+ self._partial_revision_id_to_revno_cache = {}
46
47 def _gen_revision_history(self):
48 """Return sequence of revision hashes on to this branch.
49@@ -831,15 +855,20 @@
50 except ValueError:
51 raise errors.NoSuchRevision(self, revision_id)
52
53+ @needs_read_lock
54 def get_rev_id(self, revno, history=None):
55 """Find the revision id of the specified revno."""
56 if revno == 0:
57 return _mod_revision.NULL_REVISION
58- if history is None:
59- history = self.revision_history()
60- if revno <= 0 or revno > len(history):
61+ last_revno, last_revid = self.last_revision_info()
62+ if revno == last_revno:
63+ return last_revid
64+ if revno <= 0 or revno > last_revno:
65 raise errors.NoSuchRevision(self, revno)
66- return history[revno - 1]
67+ distance_from_last = last_revno - revno
68+ if len(self._partial_revision_history_cache) <= distance_from_last:
69+ self._extend_partial_history(revno)
70+ return self._partial_revision_history_cache[revno]
71
72 @needs_write_lock
73 def pull(self, source, overwrite=False, stop_revision=None,
74@@ -2376,13 +2405,11 @@
75 self._ignore_fallbacks = kwargs.get('ignore_fallbacks', False)
76 super(BzrBranch8, self).__init__(*args, **kwargs)
77 self._last_revision_info_cache = None
78- self._partial_revision_history_cache = []
79 self._reference_info = None
80
81 def _clear_cached_state(self):
82 super(BzrBranch8, self)._clear_cached_state()
83 self._last_revision_info_cache = None
84- self._partial_revision_history_cache = []
85 self._reference_info = None
86
87 def _last_revision_info(self):
88@@ -2444,35 +2471,6 @@
89 self._extend_partial_history(stop_index=last_revno-1)
90 return list(reversed(self._partial_revision_history_cache))
91
92- def _extend_partial_history(self, stop_index=None, stop_revision=None):
93- """Extend the partial history to include a given index
94-
95- If a stop_index is supplied, stop when that index has been reached.
96- If a stop_revision is supplied, stop when that revision is
97- encountered. Otherwise, stop when the beginning of history is
98- reached.
99-
100- :param stop_index: The index which should be present. When it is
101- present, history extension will stop.
102- :param revision_id: The revision id which should be present. When
103- it is encountered, history extension will stop.
104- """
105- repo = self.repository
106- if len(self._partial_revision_history_cache) == 0:
107- iterator = repo.iter_reverse_revision_history(self.last_revision())
108- else:
109- start_revision = self._partial_revision_history_cache[-1]
110- iterator = repo.iter_reverse_revision_history(start_revision)
111- #skip the last revision in the list
112- next_revision = iterator.next()
113- for revision_id in iterator:
114- self._partial_revision_history_cache.append(revision_id)
115- if (stop_index is not None and
116- len(self._partial_revision_history_cache) > stop_index):
117- break
118- if revision_id == stop_revision:
119- break
120-
121 def _write_revision_history(self, history):
122 """Factored out of set_revision_history.
123
124
125=== modified file 'bzrlib/builtins.py'
126--- bzrlib/builtins.py 2009-06-11 06:54:33 +0000
127+++ bzrlib/builtins.py 2009-06-11 19:35:21 +0000
128@@ -946,29 +946,36 @@
129 if branch_to.get_parent() is None or remember:
130 branch_to.set_parent(branch_from.base)
131
132- if revision is not None:
133- revision_id = revision.as_revision_id(branch_from)
134-
135- branch_to.lock_write()
136+ if branch_from is not branch_to:
137+ branch_from.lock_read()
138 try:
139- if tree_to is not None:
140- view_info = _get_view_info_for_change_reporter(tree_to)
141- change_reporter = delta._ChangeReporter(
142- unversioned_filter=tree_to.is_ignored, view_info=view_info)
143- result = tree_to.pull(branch_from, overwrite, revision_id,
144- change_reporter,
145- possible_transports=possible_transports,
146- local=local)
147- else:
148- result = branch_to.pull(branch_from, overwrite, revision_id,
149- local=local)
150-
151- result.report(self.outf)
152- if verbose and result.old_revid != result.new_revid:
153- log.show_branch_change(branch_to, self.outf, result.old_revno,
154- result.old_revid)
155+ if revision is not None:
156+ revision_id = revision.as_revision_id(branch_from)
157+
158+ branch_to.lock_write()
159+ try:
160+ if tree_to is not None:
161+ view_info = _get_view_info_for_change_reporter(tree_to)
162+ change_reporter = delta._ChangeReporter(
163+ unversioned_filter=tree_to.is_ignored,
164+ view_info=view_info)
165+ result = tree_to.pull(
166+ branch_from, overwrite, revision_id, change_reporter,
167+ possible_transports=possible_transports, local=local)
168+ else:
169+ result = branch_to.pull(
170+ branch_from, overwrite, revision_id, local=local)
171+
172+ result.report(self.outf)
173+ if verbose and result.old_revid != result.new_revid:
174+ log.show_branch_change(
175+ branch_to, self.outf, result.old_revno,
176+ result.old_revid)
177+ finally:
178+ branch_to.unlock()
179 finally:
180- branch_to.unlock()
181+ if branch_from is not branch_to:
182+ branch_from.unlock()
183
184
185 class cmd_push(Command):
186
187=== modified file 'bzrlib/remote.py'
188--- bzrlib/remote.py 2009-06-11 09:11:21 +0000
189+++ bzrlib/remote.py 2009-06-11 19:35:22 +0000
190@@ -28,12 +28,10 @@
191 errors,
192 graph,
193 lockdir,
194- pack,
195 repository,
196 revision,
197 revision as _mod_revision,
198 symbol_versioning,
199- urlutils,
200 )
201 from bzrlib.branch import BranchReferenceFormat
202 from bzrlib.bzrdir import BzrDir, RemoteBzrDirFormat
203@@ -675,6 +673,24 @@
204 return self._real_repository.get_missing_parent_inventories(
205 check_for_missing_texts=check_for_missing_texts)
206
207+ def get_rev_id_for_revno(self, revno, known_pair):
208+ """See Repository.get_rev_id_for_revno."""
209+ path = self.bzrdir._path_for_remote_call(self._client)
210+ try:
211+ response = self._call(
212+ 'Repository.get_rev_id_for_revno', path, revno, known_pair)
213+ except errors.UnknownSmartMethod:
214+ self._client.medium._remember_remote_is_before((1, 16))
215+ self._ensure_real()
216+ return self._real_repository.get_rev_id_for_revno(
217+ revno, known_pair)
218+ if response[0] == 'ok':
219+ return True, response[1]
220+ elif response[0] == 'history-incomplete':
221+ return False, response[1:3]
222+ else:
223+ raise errors.UnexpectedSmartServerResponse(response)
224+
225 def _ensure_real(self):
226 """Ensure that there is a _real_repository set.
227
228@@ -1919,11 +1935,6 @@
229 # We intentionally don't call the parent class's __init__, because it
230 # will try to assign to self.tags, which is a property in this subclass.
231 # And the parent's __init__ doesn't do much anyway.
232- self._revision_id_to_revno_cache = None
233- self._partial_revision_id_to_revno_cache = {}
234- self._revision_history_cache = None
235- self._last_revision_info_cache = None
236- self._merge_sorted_revisions_cache = None
237 self.bzrdir = remote_bzrdir
238 if _client is not None:
239 self._client = _client
240@@ -1943,6 +1954,7 @@
241 else:
242 self._real_branch = None
243 # Fill out expected attributes of branch for bzrlib API users.
244+ self._clear_cached_state()
245 self.base = self.bzrdir.root_transport.base
246 self._control_files = None
247 self._lock_mode = None
248@@ -2229,6 +2241,17 @@
249 raise NotImplementedError(self.dont_leave_lock_in_place)
250 self._leave_lock = False
251
252+ def get_rev_id(self, revno, history=None):
253+ last_revision_info = self.last_revision_info()
254+ result = last_revision_info
255+ for repo in [self.repository] + self.repository._fallback_repositories:
256+ ok, result = repo.get_rev_id_for_revno(revno, result)
257+ if ok:
258+ return result
259+ closest = result[1]
260+ missing_parent = self.repository.get_parent_map([closest])[closest][0]
261+ raise errors.RevisionNotPresent(missing_parent, self.repository)
262+
263 def _last_revision_info(self):
264 response = self._call('Branch.last_revision_info', self._remote_path())
265 if response[0] != 'ok':
266
267=== modified file 'bzrlib/repository.py'
268--- bzrlib/repository.py 2009-06-10 03:56:49 +0000
269+++ bzrlib/repository.py 2009-06-11 19:35:22 +0000
270@@ -2234,6 +2234,41 @@
271 """
272 return self.get_revision(revision_id).inventory_sha1
273
274+ def get_rev_id_for_revno(self, revno, known_pair):
275+ """Return the revision id of a revno, given a later (revno, revid)
276+ pair in the same history.
277+
278+ :return: if found (True, revid). If the available history ran out
279+ before reaching the revno, then this returns
280+ (False, (closest_revno, closest_revid)).
281+ """
282+ known_revno, known_revid = known_pair
283+ partial_history = [known_revid]
284+ distance_from_known = known_revno - revno
285+ if distance_from_known < 0:
286+ raise ValueError(
287+ 'requested revno (%d) is later than given known revno (%d)'
288+ % (revno, known_revno))
289+ # XXX: handle distance_from_known < 0, etc
290+ try:
291+ _iter_for_revno(self, partial_history, stop_index=revno)
292+ except errors.RevisionNotPresent, err:
293+ if err.revision_id == known_revid:
294+ # The start revision (known_revid) wasn't found.
295+ raise
296+ # This is a stacked repository with no fallbacks, or a there's a
297+ # left-hand ghost. Either way, even though the revision named in
298+ # the error isn't in this repo, we know it's the next step in this
299+ # left-hand history.
300+ partial_history.append(err.revision_id)
301+ if len(partial_history) <= distance_from_known:
302+ # Didn't find enough history to get a revid for the revno.
303+ earliest_revno = known_revno - len(partial_history) + 1
304+ return (False, (earliest_revno, partial_history[-1]))
305+ if len(partial_history) - 1 > distance_from_known:
306+ raise AssertionError('_iter_for_revno returned too much history')
307+ return (True, partial_history[-1])
308+
309 def iter_reverse_revision_history(self, revision_id):
310 """Iterate backwards through revision ids in the lefthand history
311
312@@ -4412,3 +4447,35 @@
313 yield versionedfile.FulltextContentFactory(
314 key, parent_keys, None, as_bytes)
315
316+
317+def _iter_for_revno(repo, partial_history_cache, stop_index=None,
318+ stop_revision=None):
319+ """Extend the partial history to include a given index
320+
321+ If a stop_index is supplied, stop when that index has been reached.
322+ If a stop_revision is supplied, stop when that revision is
323+ encountered. Otherwise, stop when the beginning of history is
324+ reached.
325+
326+ :param stop_index: The index which should be present. When it is
327+ present, history extension will stop.
328+ :param stop_revision: The revision id which should be present. When
329+ it is encountered, history extension will stop.
330+ """
331+ start_revision = partial_history_cache[-1]
332+ iterator = repo.iter_reverse_revision_history(start_revision)
333+ try:
334+ #skip the last revision in the list
335+ next_revision = iterator.next()
336+ while True:
337+ if (stop_index is not None and
338+ len(partial_history_cache) > stop_index):
339+ break
340+ revision_id = iterator.next()
341+ partial_history_cache.append(revision_id)
342+ if revision_id == stop_revision:
343+ break
344+ except StopIteration:
345+ # No more history
346+ return
347+
348
349=== modified file 'bzrlib/smart/repository.py'
350--- bzrlib/smart/repository.py 2009-06-10 03:56:49 +0000
351+++ bzrlib/smart/repository.py 2009-06-11 19:35:22 +0000
352@@ -19,7 +19,6 @@
353 import bz2
354 import os
355 import Queue
356-import struct
357 import sys
358 import tarfile
359 import tempfile
360@@ -284,6 +283,31 @@
361 return SuccessfulSmartServerResponse(('ok', ), '\n'.join(lines))
362
363
364+class SmartServerRepositoryGetRevIdForRevno(SmartServerRepositoryReadLocked):
365+
366+ def do_readlocked_repository_request(self, repository, revno,
367+ known_pair):
368+ """Find the revid for a given revno, given a known revno/revid pair.
369+
370+ New in 1.16.
371+ """
372+ try:
373+ found_flag, result = repository.get_rev_id_for_revno(revno, known_pair)
374+ except errors.RevisionNotPresent, err:
375+ if err.revision_id != known_pair[1]:
376+ raise AssertionError(
377+ 'get_rev_id_for_revno raised RevisionNotPresent for '
378+ 'non-initial revision: ' + err.revision_id)
379+ return FailedSmartServerResponse(
380+ ('nosuchrevision', err.revision_id))
381+ if found_flag:
382+ return SuccessfulSmartServerResponse(('ok', result))
383+ else:
384+ earliest_revno, earliest_revid = result
385+ return SuccessfulSmartServerResponse(
386+ ('history-incomplete', earliest_revno, earliest_revid))
387+
388+
389 class SmartServerRequestHasRevision(SmartServerRepositoryRequest):
390
391 def do_repository_request(self, repository, revision_id):
392
393=== modified file 'bzrlib/smart/request.py'
394--- bzrlib/smart/request.py 2009-04-24 05:08:51 +0000
395+++ bzrlib/smart/request.py 2009-06-11 19:35:22 +0000
396@@ -555,6 +555,9 @@
397 request_handlers.register_lazy(
398 'Repository.unlock', 'bzrlib.smart.repository', 'SmartServerRepositoryUnlock')
399 request_handlers.register_lazy(
400+ 'Repository.get_rev_id_for_revno', 'bzrlib.smart.repository',
401+ 'SmartServerRepositoryGetRevIdForRevno')
402+request_handlers.register_lazy(
403 'Repository.get_stream', 'bzrlib.smart.repository',
404 'SmartServerRepositoryGetStream')
405 request_handlers.register_lazy(
406
407=== modified file 'bzrlib/tests/blackbox/test_pull.py'
408--- bzrlib/tests/blackbox/test_pull.py 2009-06-11 07:43:56 +0000
409+++ bzrlib/tests/blackbox/test_pull.py 2009-06-11 19:35:22 +0000
410@@ -368,8 +368,12 @@
411 See <https://launchpad.net/bugs/380314>
412 """
413 self.setup_smart_server_with_call_log()
414+ # Make a stacked-on branch with two commits so that the
415+ # revision-history can't be determined just by looking at the parent
416+ # field in the revision in the stacked repo.
417 parent = self.make_branch_and_tree('parent', format='1.9')
418 parent.commit(message='first commit')
419+ parent.commit(message='second commit')
420 local = parent.bzrdir.sprout('local').open_workingtree()
421 local.commit(message='local commit')
422 local.branch.create_clone_on_transport(
423@@ -383,7 +387,7 @@
424 # being too low. If rpc_count increases, more network roundtrips have
425 # become necessary for this use case. Please do not adjust this number
426 # upwards without agreement from bzr's network support maintainers.
427- self.assertLength(43, self.hpss_calls)
428+ self.assertLength(18, self.hpss_calls)
429 remote = Branch.open('stacked')
430 self.assertEndsWith(remote.get_stacked_on_url(), '/parent')
431
432
433=== modified file 'bzrlib/tests/test_smart.py'
434--- bzrlib/tests/test_smart.py 2009-06-11 07:10:44 +0000
435+++ bzrlib/tests/test_smart.py 2009-06-11 19:35:22 +0000
436@@ -1161,6 +1161,47 @@
437 request.execute('', 'missingrevision'))
438
439
440+class TestSmartServerRepositoryGetRevIdForRevno(tests.TestCaseWithMemoryTransport):
441+
442+ def test_revno_found(self):
443+ backing = self.get_transport()
444+ request = smart.repository.SmartServerRepositoryGetRevIdForRevno(backing)
445+ tree = self.make_branch_and_memory_tree('.')
446+ tree.lock_write()
447+ tree.add('')
448+ rev1_id_utf8 = u'\xc8'.encode('utf-8')
449+ rev2_id_utf8 = u'\xc9'.encode('utf-8')
450+ tree.commit('1st commit', rev_id=rev1_id_utf8)
451+ tree.commit('2nd commit', rev_id=rev2_id_utf8)
452+ tree.unlock()
453+
454+ self.assertEqual(SmartServerResponse(('ok', rev1_id_utf8)),
455+ request.execute('', 1, (2, rev2_id_utf8)))
456+
457+ def test_known_revid_missing(self):
458+ backing = self.get_transport()
459+ request = smart.repository.SmartServerRepositoryGetRevIdForRevno(backing)
460+ repo = self.make_repository('.')
461+ self.assertEqual(
462+ FailedSmartServerResponse(('nosuchrevision', 'ghost')),
463+ request.execute('', 1, (2, 'ghost')))
464+
465+ def test_history_incomplete(self):
466+ backing = self.get_transport()
467+ request = smart.repository.SmartServerRepositoryGetRevIdForRevno(backing)
468+ parent = self.make_branch_and_memory_tree('parent', format='1.9')
469+ r1 = parent.commit(message='first commit')
470+ r2 = parent.commit(message='second commit')
471+ local = self.make_branch_and_memory_tree('local', format='1.9')
472+ local.branch.pull(parent.branch)
473+ local.set_parent_ids([r2])
474+ r3 = local.commit(message='local commit')
475+ local.branch.create_clone_on_transport(
476+ self.get_transport('stacked'), stacked_on=self.get_url('parent'))
477+ self.assertEqual(
478+ SmartServerResponse(('history-incomplete', 2, r2)),
479+ request.execute('stacked', 1, (3, r3)))
480+
481 class TestSmartServerRepositoryGetStream(tests.TestCaseWithMemoryTransport):
482
483 def make_two_commit_repo(self):
484@@ -1576,6 +1617,8 @@
485 smart.repository.SmartServerRepositoryGatherStats)
486 self.assertHandlerEqual('Repository.get_parent_map',
487 smart.repository.SmartServerRepositoryGetParentMap)
488+ self.assertHandlerEqual('Repository.get_rev_id_for_revno',
489+ smart.repository.SmartServerRepositoryGetRevIdForRevno)
490 self.assertHandlerEqual('Repository.get_revision_graph',
491 smart.repository.SmartServerRepositoryGetRevisionGraph)
492 self.assertHandlerEqual('Repository.get_stream',