Merge lp:~spiv/bzr/hpss-second-push-error-bug-437626 into lp:bzr/2.0

[Andrew Bennetts]

[Andrew Bennetts (spiv)]

This fixes bug 437626. The problem was that the fairly subtle requirements of add_lines(..., missing_compression_parents=True) were not being met by the way insert_record_stream was calling it multiple times (in effectively random order) for the one set of buffered index entries. The correct thing to do is call add_lines just once with all buffered entries together.

The result of the bug was that for C->B and B->A (where -> means left depends on right as a compression parent) get_missing_compression_parents would sometimes incorrectly report [B,A] rather than just [A], which then leads to fetch thinking that keys are still missing.

I feel a bit like a docstring or comment ought to be clearer about this, but really if you read carefully this requirement is already implicit, and the comments/docstrings for this part of the code are already getting towards the "eyes glaze over" point. On the other hand I've added a test so the chances of this regressing are pretty low.

[John A Meinel (jameinel)]

Looks good to me.

review: Approve

=== modified file 'NEWS'
--- NEWS	2009-10-19 05:38:30 +0000
+++ NEWS	2009-10-21 01:20:22 +0000
@@ -25,6 +25,10 @@
 
 * Diff parsing handles "Binary files differ" hunks.  (Aaron Bentley, #436325)
 
+* Fetching from stacked pre-2a repository via a smart server no longer
+  fails intermittently with "second push failed to complete".
+  (Andrew Bennetts, #437626)
+
 * PreviewTree file names are not limited by the encoding of the temp
   directory's filesystem. (Aaron Bentley, #436794)
 

=== modified file 'bzrlib/knit.py'
--- bzrlib/knit.py	2009-09-09 08:48:29 +0000
+++ bzrlib/knit.py	2009-10-21 01:20:22 +0000
@@ -1710,10 +1710,12 @@
             # There were index entries buffered at the end of the stream,
             # So these need to be added (if the index supports holding such
             # entries for later insertion)
+            all_entries = []
             for key in buffered_index_entries:
                 index_entries = buffered_index_entries[key]
-                self._index.add_records(index_entries,
-                    missing_compression_parents=True)
+                all_entries.extend(index_entries)
+            self._index.add_records(
+                all_entries, missing_compression_parents=True)
 
     def get_missing_compression_parent_keys(self):
         """Return an iterable of keys of missing compression parents.

=== modified file 'bzrlib/tests/per_versionedfile.py'
--- bzrlib/tests/per_versionedfile.py	2009-08-26 16:44:27 +0000
+++ bzrlib/tests/per_versionedfile.py	2009-10-21 01:20:22 +0000
@@ -2434,6 +2434,43 @@
         else:
             self.assertIdenticalVersionedFile(source, files)
 
+    def test_insert_record_stream_long_parent_chain_out_of_order(self):
+        """An out of order stream can either error or work."""
+        if not self.graph:
+            raise TestNotApplicable('ancestry info only relevant with graph.')
+        # Create a reasonably long chain of records based on each other, where
+        # most will be deltas.
+        source = self.get_versionedfiles('source')
+        parents = ()
+        keys = []
+        content = [('same same %d\n' % n) for n in range(500)]
+        for letter in 'abcdefghijklmnopqrstuvwxyz':
+            key = ('key-' + letter,)
+            if self.key_length == 2:
+                key = ('prefix',) + key
+            content.append('content for ' + letter + '\n')
+            source.add_lines(key, parents, content)
+            keys.append(key)
+            parents = (key,)
+        # Create a stream of these records, excluding the first record that the
+        # rest ultimately depend upon, and insert it into a new vf.
+        streams = []
+        for key in reversed(keys):
+            streams.append(source.get_record_stream([key], 'unordered', False))
+        deltas = chain(*streams[:-1])
+        files = self.get_versionedfiles()
+        try:
+            files.insert_record_stream(deltas)
+        except RevisionNotPresent:
+            # Must not have corrupted the file.
+            files.check()
+        else:
+            # Must only report either just the first key as a missing parent,
+            # no key as missing (for nodelta scenarios).
+            missing = set(files.get_missing_compression_parent_keys())
+            missing.discard(keys[0])
+            self.assertEqual(set(), missing)
+
     def get_knit_delta_source(self):
         """Get a source that can produce a stream with knit delta records,
         regardless of this test's scenario.