Merge lp:~jameinel/bzr/quick-fix into lp:~bzr/bzr/trunk-old

Proposed by John A Meinel on 2009-06-01
Status: Merged
Merged at revision: not available
Proposed branch: lp:~jameinel/bzr/quick-fix
Merge into: lp:~bzr/bzr/trunk-old
Diff against target: 252 lines
To merge this branch: bzr merge lp:~jameinel/bzr/quick-fix
Reviewer Review Type Date Requested Status
Andrew Bennetts 2009-06-01 Approve on 2009-06-02
Review via email: mp+6948@code.launchpad.net
To post a comment you must log in.
John A Meinel (jameinel) wrote :

Fixes some issues with fetch + ghosts + smart server
Turns out that my update for gc stacking slightly regressed our ability to push 'bzr.dev' via the smart server, because of how missing inventories and ghosts interact.

This patch adds tests that assert we can fetch via smart server for *all* formats with a revision that has a ghost parent (a test which was missing w/ Andrew's fix). It also adds similar tests for fetching revisions w/ ghosts into a stacked branch. (Which Andrew's original fix did *not* address.)

Both StreamSource and GroupCHKStreamSource were affected by this, though there are still more failures with --dev6 + stacking + smart server code to be fixed. (Namely 'bzr push bzr+ssh://.../new-branch' fails because the smart remote client tries to 'upgrade' the default branch to a hard-coded 1.6.1/1.6-rich-root format repository)

Andrew Bennetts (spiv) wrote :

I think this looks good. One change you should make: if it does fix more than my previous ghosts+stacking fix as you say, then it deserves a NEWS entry. :)

review: Approve
lp:~jameinel/bzr/quick-fix updated on 2009-06-02
4395. By John A Meinel on 2009-06-02

NEWS entry per Andrew's review

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2009-05-29 11:26:30 +0000
3+++ NEWS 2009-06-02 02:35:12 +0000
4@@ -18,6 +18,11 @@
5 Improvements
6 ************
7
8+
9+* ``--development6-rich-root`` can now stack. (Modulo some smart-server
10+ bugs with stacking and non default formats.)
11+ (John Arbash Meinel, #373455)
12+
13 * Numerous operations are now faster for huge projects, i.e. those
14 with a large number of files and/or a large number of revisions,
15 particularly when the latest development format is used. These
16@@ -30,10 +35,6 @@
17
18 (Ian Clatworthy)
19
20-* ``--development6-rich-root`` can now stack. (Modulo some smart-server
21- bugs with stacking and non default formats.)
22- (John Arbash Meinel, #373455)
23-
24
25 Bug Fixes
26 *********
27@@ -45,6 +46,11 @@
28 ``RemoteRepository`` was handling fallbacks along with the
29 ``_real_repository``. (Andrew Bennetts, John Arbash Meinel, #375496)
30
31+* Fix a small bug with fetching revisions w/ ghosts into a new stacked
32+ branch. Not often triggered, because it required ghosts to be part of
33+ the fetched revisions, not in the stacked-on ancestry.
34+ (John Arbash Meinel)
35+
36
37 Documentation
38 *************
39
40=== modified file 'bzrlib/repofmt/groupcompress_repo.py'
41--- bzrlib/repofmt/groupcompress_repo.py 2009-05-29 10:25:37 +0000
42+++ bzrlib/repofmt/groupcompress_repo.py 2009-06-02 02:35:12 +0000
43@@ -857,7 +857,7 @@
44 self._chk_id_roots = None
45 self._chk_p_id_roots = None
46
47- def _get_inventory_stream(self, inventory_keys):
48+ def _get_inventory_stream(self, inventory_keys, allow_absent=False):
49 """Get a stream of inventory texts.
50
51 When this function returns, self._chk_id_roots and self._chk_p_id_roots
52@@ -872,6 +872,11 @@
53 stream = source_vf.get_record_stream(inventory_keys,
54 'groupcompress', True)
55 for record in stream:
56+ if record.storage_kind == 'absent':
57+ if allow_absent:
58+ continue
59+ else:
60+ raise errors.NoSuchRevision(self, record.key)
61 bytes = record.get_bytes_as('fulltext')
62 chk_inv = inventory.CHKInventory.deserialise(None, bytes,
63 record.key)
64@@ -981,7 +986,11 @@
65 raise AssertionError('Cannot call get_stream_for_missing_keys'
66 ' untill all of get_stream() has been consumed.')
67 # Yield the inventory stream, so we can find the chk stream
68- yield self._get_inventory_stream(missing_inventory_keys)
69+ # Some of the missing_keys will be missing because they are ghosts.
70+ # As such, we can ignore them. The Sink is required to verify there are
71+ # no unavailable texts when the ghost inventories are not filled in.
72+ yield self._get_inventory_stream(missing_inventory_keys,
73+ allow_absent=True)
74 # We use the empty set for excluded_revision_ids, to make it clear that
75 # we want to transmit all referenced chk pages.
76 for stream_info in self._get_filtered_chk_streams(set()):
77
78=== modified file 'bzrlib/repository.py'
79--- bzrlib/repository.py 2009-05-27 15:39:12 +0000
80+++ bzrlib/repository.py 2009-06-02 02:35:12 +0000
81@@ -4265,8 +4265,12 @@
82 continue
83 # Ask for full texts always so that we don't need more round trips
84 # after this stream.
85- stream = vf.get_record_stream(keys,
86- self.to_format._fetch_order, True)
87+ # Some of the missing keys are genuinely ghosts, so filter absent
88+ # records. The Sink is responsible for doing another check to
89+ # ensure that ghosts don't introduce missing data for future
90+ # fetches.
91+ stream = versionedfile.filter_absent(vf.get_record_stream(keys,
92+ self.to_format._fetch_order, True))
93 yield substream_kind, stream
94
95 def inventory_fetch_order(self):
96
97=== modified file 'bzrlib/smart/repository.py'
98--- bzrlib/smart/repository.py 2009-05-07 05:08:46 +0000
99+++ bzrlib/smart/repository.py 2009-06-02 02:35:12 +0000
100@@ -436,6 +436,8 @@
101 for record in substream:
102 if record.storage_kind in ('chunked', 'fulltext'):
103 serialised = record_to_fulltext_bytes(record)
104+ elif record.storage_kind == 'absent':
105+ raise ValueError("Absent factory for %s" % (record.key,))
106 else:
107 serialised = record.get_bytes_as(record.storage_kind)
108 if serialised:
109
110=== modified file 'bzrlib/tests/per_repository/test_fetch.py'
111--- bzrlib/tests/per_repository/test_fetch.py 2009-05-23 04:55:52 +0000
112+++ bzrlib/tests/per_repository/test_fetch.py 2009-06-02 02:35:12 +0000
113@@ -23,6 +23,7 @@
114 graph,
115 remote,
116 repository,
117+ tests,
118 )
119 from bzrlib.inventory import ROOT_ID
120 from bzrlib.tests import TestSkipped
121@@ -145,7 +146,7 @@
122 ])
123
124 def test_fetch_to_rich_root_set_parent_1_parent(self):
125- # 1 parent rev -> 1 parent
126+ # 1 parent rev -> 1 parent
127 self.do_test_fetch_to_rich_root_sets_parents_correctly(
128 ((ROOT_ID, 'base'),),
129 [('base', None, [('add', ('', ROOT_ID, 'directory', ''))]),
130@@ -300,6 +301,55 @@
131 repo.fetch(tree.branch.repository)
132 repo.fetch(tree.branch.repository)
133
134+ def make_simple_branch_with_ghost(self):
135+ builder = self.make_branch_builder('source')
136+ builder.start_series()
137+ builder.build_snapshot('A-id', None, [
138+ ('add', ('', 'root-id', 'directory', None)),
139+ ('add', ('file', 'file-id', 'file', 'content\n'))])
140+ builder.build_snapshot('B-id', ['A-id', 'ghost-id'], [])
141+ builder.finish_series()
142+ source_b = builder.get_branch()
143+ source_b.lock_read()
144+ self.addCleanup(source_b.unlock)
145+ return source_b
146+
147+ def test_fetch_with_ghost(self):
148+ source_b = self.make_simple_branch_with_ghost()
149+ target = self.make_repository('target')
150+ target.lock_write()
151+ self.addCleanup(target.unlock)
152+ target.fetch(source_b.repository, revision_id='B-id')
153+
154+ def test_fetch_into_smart_with_ghost(self):
155+ trans = self.make_smart_server('target')
156+ source_b = self.make_simple_branch_with_ghost()
157+ target = self.make_repository('target')
158+ # Re-open the repository over the smart protocol
159+ target = repository.Repository.open(trans.base)
160+ target.lock_write()
161+ self.addCleanup(target.unlock)
162+ try:
163+ target.fetch(source_b.repository, revision_id='B-id')
164+ except errors.TokenLockingNotSupported:
165+ # The code inside fetch() that tries to lock and then fails, also
166+ # causes weird problems with 'lock_not_held' later on...
167+ target.lock_read()
168+ raise tests.KnownFailure('some repositories fail to fetch'
169+ ' via the smart server because of locking issues.')
170+
171+ def test_fetch_from_smart_with_ghost(self):
172+ trans = self.make_smart_server('source')
173+ source_b = self.make_simple_branch_with_ghost()
174+ target = self.make_repository('target')
175+ target.lock_write()
176+ self.addCleanup(target.unlock)
177+ # Re-open the repository over the smart protocol
178+ source = repository.Repository.open(trans.base)
179+ source.lock_read()
180+ self.addCleanup(source.unlock)
181+ target.fetch(source, revision_id='B-id')
182+
183
184 class TestSource(TestCaseWithRepository):
185 """Tests for/about the results of Repository._get_source."""
186
187=== modified file 'bzrlib/tests/per_repository_reference/test_fetch.py'
188--- bzrlib/tests/per_repository_reference/test_fetch.py 2009-05-29 09:56:12 +0000
189+++ bzrlib/tests/per_repository_reference/test_fetch.py 2009-06-02 02:35:12 +0000
190@@ -15,7 +15,13 @@
191 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
192
193
194-from bzrlib.smart import server
195+from bzrlib import (
196+ branch,
197+ errors,
198+ )
199+from bzrlib.smart import (
200+ server,
201+ )
202 from bzrlib.tests.per_repository import TestCaseWithRepository
203
204
205@@ -99,3 +105,47 @@
206 final2_b = target_b.bzrdir.sprout('final2',
207 revision_id='C-id').open_branch()
208 self.assertEqual('C-id', final_b.last_revision())
209+
210+ def make_source_with_ghost_and_stacked_target(self):
211+ builder = self.make_branch_builder('source')
212+ builder.start_series()
213+ builder.build_snapshot('A-id', None, [
214+ ('add', ('', 'root-id', 'directory', None)),
215+ ('add', ('file', 'file-id', 'file', 'content\n'))])
216+ builder.build_snapshot('B-id', ['A-id', 'ghost-id'], [])
217+ builder.finish_series()
218+ source_b = builder.get_branch()
219+ source_b.lock_read()
220+ self.addCleanup(source_b.unlock)
221+ base = self.make_branch('base')
222+ base.pull(source_b, stop_revision='A-id')
223+ stacked = self.make_branch('stacked')
224+ stacked.set_stacked_on_url('../base')
225+ return source_b, base, stacked
226+
227+ def test_fetch_with_ghost_stacked(self):
228+ (source_b, base,
229+ stacked) = self.make_source_with_ghost_and_stacked_target()
230+ stacked.pull(source_b, stop_revision='B-id')
231+
232+ def test_fetch_into_smart_stacked_with_ghost(self):
233+ (source_b, base,
234+ stacked) = self.make_source_with_ghost_and_stacked_target()
235+ # Now, create a smart server on 'stacked' and re-open to force the
236+ # target to be a smart target
237+ trans = self.make_smart_server('stacked')
238+ stacked = branch.Branch.open(trans.base)
239+ stacked.lock_write()
240+ self.addCleanup(stacked.unlock)
241+ stacked.pull(source_b, stop_revision='B-id')
242+
243+ def test_fetch_to_stacked_from_smart_with_ghost(self):
244+ (source_b, base,
245+ stacked) = self.make_source_with_ghost_and_stacked_target()
246+ # Now, create a smart server on 'source' and re-open to force the
247+ # target to be a smart target
248+ trans = self.make_smart_server('source')
249+ source_b = branch.Branch.open(trans.base)
250+ source_b.lock_read()
251+ self.addCleanup(source_b.unlock)
252+ stacked.pull(source_b, stop_revision='B-id')