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

[John A Meinel]

[John A Meinel (jameinel)]

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)]

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

=== modified file 'NEWS'
--- NEWS	2009-05-29 11:26:30 +0000
+++ NEWS	2009-06-02 02:35:12 +0000
@@ -18,6 +18,11 @@
 Improvements
 ************
 
+
+* ``--development6-rich-root`` can now stack. (Modulo some smart-server
+  bugs with stacking and non default formats.)
+  (John Arbash Meinel, #373455)
+
 * Numerous operations are now faster for huge projects, i.e. those
   with a large number of files and/or a large number of revisions,
   particularly when the latest development format is used. These
@@ -30,10 +35,6 @@
 
   (Ian Clatworthy)
 
-* ``--development6-rich-root`` can now stack. (Modulo some smart-server
-  bugs with stacking and non default formats.)
-  (John Arbash Meinel, #373455)
-
 
 Bug Fixes
 *********
@@ -45,6 +46,11 @@
   ``RemoteRepository`` was handling fallbacks along with the
   ``_real_repository``. (Andrew Bennetts, John Arbash Meinel, #375496)
 
+* Fix a small bug with fetching revisions w/ ghosts into a new stacked
+  branch. Not often triggered, because it required ghosts to be part of
+  the fetched revisions, not in the stacked-on ancestry.
+  (John Arbash Meinel)
+
 
 Documentation
 *************

=== modified file 'bzrlib/repofmt/groupcompress_repo.py'
--- bzrlib/repofmt/groupcompress_repo.py	2009-05-29 10:25:37 +0000
+++ bzrlib/repofmt/groupcompress_repo.py	2009-06-02 02:35:12 +0000
@@ -857,7 +857,7 @@
         self._chk_id_roots = None
         self._chk_p_id_roots = None
 
-    def _get_inventory_stream(self, inventory_keys):
+    def _get_inventory_stream(self, inventory_keys, allow_absent=False):
         """Get a stream of inventory texts.
 
         When this function returns, self._chk_id_roots and self._chk_p_id_roots
@@ -872,6 +872,11 @@
             stream = source_vf.get_record_stream(inventory_keys,
                                                  'groupcompress', True)
             for record in stream:
+                if record.storage_kind == 'absent':
+                    if allow_absent:
+                        continue
+                    else:
+                        raise errors.NoSuchRevision(self, record.key)
                 bytes = record.get_bytes_as('fulltext')
                 chk_inv = inventory.CHKInventory.deserialise(None, bytes,
                                                              record.key)
@@ -981,7 +986,11 @@
             raise AssertionError('Cannot call get_stream_for_missing_keys'
                 ' untill all of get_stream() has been consumed.')
         # Yield the inventory stream, so we can find the chk stream
-        yield self._get_inventory_stream(missing_inventory_keys)
+        # Some of the missing_keys will be missing because they are ghosts.
+        # As such, we can ignore them. The Sink is required to verify there are
+        # no unavailable texts when the ghost inventories are not filled in.
+        yield self._get_inventory_stream(missing_inventory_keys,
+                                         allow_absent=True)
         # We use the empty set for excluded_revision_ids, to make it clear that
         # we want to transmit all referenced chk pages.
         for stream_info in self._get_filtered_chk_streams(set()):

=== modified file 'bzrlib/repository.py'
--- bzrlib/repository.py	2009-05-27 15:39:12 +0000
+++ bzrlib/repository.py	2009-06-02 02:35:12 +0000
@@ -4265,8 +4265,12 @@
                 continue
             # Ask for full texts always so that we don't need more round trips
             # after this stream.
-            stream = vf.get_record_stream(keys,
-                self.to_format._fetch_order, True)
+            # Some of the missing keys are genuinely ghosts, so filter absent
+            # records. The Sink is responsible for doing another check to
+            # ensure that ghosts don't introduce missing data for future
+            # fetches.
+            stream = versionedfile.filter_absent(vf.get_record_stream(keys,
+                self.to_format._fetch_order, True))
             yield substream_kind, stream
 
     def inventory_fetch_order(self):

=== modified file 'bzrlib/smart/repository.py'
--- bzrlib/smart/repository.py	2009-05-07 05:08:46 +0000
+++ bzrlib/smart/repository.py	2009-06-02 02:35:12 +0000
@@ -436,6 +436,8 @@
         for record in substream:
             if record.storage_kind in ('chunked', 'fulltext'):
                 serialised = record_to_fulltext_bytes(record)
+            elif record.storage_kind == 'absent':
+                raise ValueError("Absent factory for %s" % (record.key,))
             else:
                 serialised = record.get_bytes_as(record.storage_kind)
             if serialised:

=== modified file 'bzrlib/tests/per_repository/test_fetch.py'
--- bzrlib/tests/per_repository/test_fetch.py	2009-05-23 04:55:52 +0000
+++ bzrlib/tests/per_repository/test_fetch.py	2009-06-02 02:35:12 +0000
@@ -23,6 +23,7 @@
     graph,
     remote,
     repository,
+    tests,
     )
 from bzrlib.inventory import ROOT_ID
 from bzrlib.tests import TestSkipped
@@ -145,7 +146,7 @@
             ])
 
     def test_fetch_to_rich_root_set_parent_1_parent(self):
-        # 1 parent rev -> 1 parent 
+        # 1 parent rev -> 1 parent
         self.do_test_fetch_to_rich_root_sets_parents_correctly(
             ((ROOT_ID, 'base'),),
             [('base', None, [('add', ('', ROOT_ID, 'directory', ''))]),
@@ -300,6 +301,55 @@
         repo.fetch(tree.branch.repository)
         repo.fetch(tree.branch.repository)
 
+    def make_simple_branch_with_ghost(self):
+        builder = self.make_branch_builder('source')
+        builder.start_series()
+        builder.build_snapshot('A-id', None, [
+            ('add', ('', 'root-id', 'directory', None)),
+            ('add', ('file', 'file-id', 'file', 'content\n'))])
+        builder.build_snapshot('B-id', ['A-id', 'ghost-id'], [])
+        builder.finish_series()
+        source_b = builder.get_branch()
+        source_b.lock_read()
+        self.addCleanup(source_b.unlock)
+        return source_b
+
+    def test_fetch_with_ghost(self):
+        source_b = self.make_simple_branch_with_ghost()
+        target = self.make_repository('target')
+        target.lock_write()
+        self.addCleanup(target.unlock)
+        target.fetch(source_b.repository, revision_id='B-id')
+
+    def test_fetch_into_smart_with_ghost(self):
+        trans = self.make_smart_server('target')
+        source_b = self.make_simple_branch_with_ghost()
+        target = self.make_repository('target')
+        # Re-open the repository over the smart protocol
+        target = repository.Repository.open(trans.base)
+        target.lock_write()
+        self.addCleanup(target.unlock)
+        try:
+            target.fetch(source_b.repository, revision_id='B-id')
+        except errors.TokenLockingNotSupported:
+            # The code inside fetch() that tries to lock and then fails, also
+            # causes weird problems with 'lock_not_held' later on...
+            target.lock_read()
+            raise tests.KnownFailure('some repositories fail to fetch'
+                ' via the smart server because of locking issues.')
+
+    def test_fetch_from_smart_with_ghost(self):
+        trans = self.make_smart_server('source')
+        source_b = self.make_simple_branch_with_ghost()
+        target = self.make_repository('target')
+        target.lock_write()
+        self.addCleanup(target.unlock)
+        # Re-open the repository over the smart protocol
+        source = repository.Repository.open(trans.base)
+        source.lock_read()
+        self.addCleanup(source.unlock)
+        target.fetch(source, revision_id='B-id')
+
 
 class TestSource(TestCaseWithRepository):
     """Tests for/about the results of Repository._get_source."""

=== modified file 'bzrlib/tests/per_repository_reference/test_fetch.py'
--- bzrlib/tests/per_repository_reference/test_fetch.py	2009-05-29 09:56:12 +0000
+++ bzrlib/tests/per_repository_reference/test_fetch.py	2009-06-02 02:35:12 +0000
@@ -15,7 +15,13 @@
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
 
 
-from bzrlib.smart import server
+from bzrlib import (
+    branch,
+    errors,
+    )
+from bzrlib.smart import (
+    server,
+    )
 from bzrlib.tests.per_repository import TestCaseWithRepository
 
 
@@ -99,3 +105,47 @@
         final2_b = target_b.bzrdir.sprout('final2',
                                           revision_id='C-id').open_branch()
         self.assertEqual('C-id', final_b.last_revision())
+
+    def make_source_with_ghost_and_stacked_target(self):
+        builder = self.make_branch_builder('source')
+        builder.start_series()
+        builder.build_snapshot('A-id', None, [
+            ('add', ('', 'root-id', 'directory', None)),
+            ('add', ('file', 'file-id', 'file', 'content\n'))])
+        builder.build_snapshot('B-id', ['A-id', 'ghost-id'], [])
+        builder.finish_series()
+        source_b = builder.get_branch()
+        source_b.lock_read()
+        self.addCleanup(source_b.unlock)
+        base = self.make_branch('base')
+        base.pull(source_b, stop_revision='A-id')
+        stacked = self.make_branch('stacked')
+        stacked.set_stacked_on_url('../base')
+        return source_b, base, stacked
+
+    def test_fetch_with_ghost_stacked(self):
+        (source_b, base,
+         stacked) = self.make_source_with_ghost_and_stacked_target()
+        stacked.pull(source_b, stop_revision='B-id')
+
+    def test_fetch_into_smart_stacked_with_ghost(self):
+        (source_b, base,
+         stacked) = self.make_source_with_ghost_and_stacked_target()
+        # Now, create a smart server on 'stacked' and re-open to force the
+        # target to be a smart target
+        trans = self.make_smart_server('stacked')
+        stacked = branch.Branch.open(trans.base)
+        stacked.lock_write()
+        self.addCleanup(stacked.unlock)
+        stacked.pull(source_b, stop_revision='B-id')
+
+    def test_fetch_to_stacked_from_smart_with_ghost(self):
+        (source_b, base,
+         stacked) = self.make_source_with_ghost_and_stacked_target()
+        # Now, create a smart server on 'source' and re-open to force the
+        # target to be a smart target
+        trans = self.make_smart_server('source')
+        source_b = branch.Branch.open(trans.base)
+        source_b.lock_read()
+        self.addCleanup(source_b.unlock)
+        stacked.pull(source_b, stop_revision='B-id')