Merge lp:~abentley/bzr/fix-shelf into lp:~bzr/bzr/trunk-old

[Aaron Bentley]

[Aaron Bentley (abentley)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi all,

This fixes a bug in the ContainerPushParser, which indrectly causes
unshelve to crash when unshelving zero-length files.

The problem is that ContainerPushParser uses self._buffer as an
indicator of progress. It assumes that if self._buffer did not change,
no progress could be made in parsing records. But in the case of a
zero-length record, _state_expecting_body will move on to the next state
 handler, without changing the length of self._buffer. This is clearly
progress.

The result of this bug is that the parser stops at the first zero-length
record, which causes TreeTransform.deserialize to produce an incomplete
TreeTransform, which causes PreviewTree.stored_kind to raise an exception.

This patch simply augments test for progress by requiring that
self._buffer have a different length, OR that the state_handler be changed.

I am inclined to think that it would be more robust to have a flag
indicating whether progress occurred, or even to raise an exception when
progress ceases.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAko8O6sACgkQ0F+nu1YWqI1LkQCgh6KV6lZd0ps9wBS2eBa+R743
BH0An25+SE9z1JcxgkZYK3rhtUyot9Dz
=WEdZ
-----END PGP SIGNATURE-----

[Andrew Bennetts (spiv)]

The code and test makes sense. Thanks for fixing my code :)

review: Approve

=== modified file 'bzrlib/pack.py'
--- bzrlib/pack.py	2009-04-04 02:50:01 +0000
+++ bzrlib/pack.py	2009-06-20 01:17:38 +0000
@@ -406,8 +406,11 @@
         # the buffer.
         last_buffer_length = None
         cur_buffer_length = len(self._buffer)
-        while cur_buffer_length != last_buffer_length:
+        last_state_handler = None
+        while (cur_buffer_length != last_buffer_length
+               or last_state_handler != self._state_handler):
             last_buffer_length = cur_buffer_length
+            last_state_handler = self._state_handler
             self._state_handler()
             cur_buffer_length = len(self._buffer)
 

=== modified file 'bzrlib/tests/test_pack.py'
--- bzrlib/tests/test_pack.py	2009-03-23 14:59:43 +0000
+++ bzrlib/tests/test_pack.py	2009-06-20 01:17:38 +0000
@@ -610,6 +610,19 @@
             [([('name1',)], 'body1'), ([('name2',)], 'body2')],
             parser.read_pending_records())
 
+    def test_multiple_empty_records_at_once(self):
+        """If multiple empty records worth of data are fed to the parser in one
+        string, the parser will correctly parse all the records.
+
+        (A naive implementation might stop after parsing the first empty
+        record, because the buffer size had not changed.)
+        """
+        parser = self.make_parser_expecting_record_type()
+        parser.accept_bytes("B0\nname1\n\nB0\nname2\n\n")
+        self.assertEqual(
+            [([('name1',)], ''), ([('name2',)], '')],
+            parser.read_pending_records())
+
 
 class TestContainerPushParserBytesParsing(PushParserTestCase):
     """Tests for reading Bytes records with ContainerPushParser.