Add test and fix for the redundant pack bug.

Also remove some unused imports and a somewhat irrelevant comment.

src/ZODB/FileStorage/fspack.py

@@ -24,17 +24,10 @@ from the revision of the root at that time or if it is reachable from
 a backpointer after that time.
 """
 
-# This module contains code backported from ZODB4 from the
-# zodb.storage.file package.  It's been edited heavily to work with
-# ZODB3 code and storage layout.
-
 import os
-import struct
-from types import StringType
 
 from ZODB.referencesf import referencesf
-from ZODB.utils import p64, u64, z64, oid_repr
-from zLOG import LOG, BLATHER, WARNING, ERROR, PANIC
+from ZODB.utils import p64, u64, z64
 
 from ZODB.fsIndex import fsIndex
 from ZODB.FileStorage.format \
@@ -232,11 +225,19 @@ class GC(FileStorageFormatter):
 
     def buildPackIndex(self):
         pos = 4L
+        # We make the initial assumption that the database has been
+        # packed before and set unpacked to True only after seeing the
+        # first record with a status == " ".  If we get to the packtime
+        # and unpacked is still False, we need to watch for a redundant
+        # pack.
+        unpacked = False
         while pos < self.eof:
             th = self._read_txn_header(pos)
             if th.tid > self.packtime:
                 break
             self.checkTxn(th, pos)
+            if th.status != "p":
+                unpacked = True
 
             tpos = pos
             end = pos + th.tlen
@@ -260,6 +261,25 @@ class GC(FileStorageFormatter):
 
         self.packpos = pos
 
+        if unpacked:
+            return
+        # check for a redundant pack.  If the first record following
+        # the newly computed packpos has status 'p', then it was
+        # packed earlier and the current pack is redudant.
+        try:
+            th = self._read_txn_header(pos)
+        except CorruptedDataError, err:
+            if err.buf != "": 
+                raise
+        if th.status == 'p':
+            # Delay import to code with circular imports.
+            # XXX put exceptions in a separate module
+            from ZODB.FileStorage.FileStorage import FileStorageError
+            print "Yow!"
+            raise FileStorageError(
+                "The database has already been packed to a later time"
+                " or no changes have been made since the last pack")
+
     def findReachableAtPacktime(self, roots):
         """Mark all objects reachable from the oids in roots as reachable."""
         todo = list(roots)
@@ -645,3 +665,4 @@ class FileStoragePacker(FileStorageFormatter):
         if self._lock_counter % 20 == 0:
             self._commit_lock_acquire()
         return ipos
+

src/ZODB/tests/PackableStorage.py

@@ -29,10 +29,11 @@ import time
 
 from ZODB import DB
 from persistent import Persistent
+from persistent.mapping import PersistentMapping
 from ZODB.referencesf import referencesf
 from ZODB.tests.MinPO import MinPO
 from ZODB.tests.StorageTestBase import snooze
-from ZODB.POSException import ConflictError
+from ZODB.POSException import ConflictError, StorageError
 
 from ZODB.tests.MTStorage import TestThread
 
@@ -126,11 +127,10 @@ class PackableStorageBase:
         try:
             self._storage.load(ZERO, '')
         except KeyError:
-            from persistent import mapping
             from ZODB.Transaction import Transaction
             file = StringIO()
             p = cPickle.Pickler(file, 1)
-            p.dump((mapping.PersistentMapping, None))
+            p.dump((PersistentMapping, None))
             p.dump({'_container': {}})
             t=Transaction()
             t.description='initial database creation'
@@ -438,6 +438,50 @@ class PackableUndoStorage(PackableStorageBase):
 
         eq(root['obj'].value, 7)
 
+    def checkRedundantPack(self):
+        # It is an error to perform a pack with a packtime earlier
+        # than a previous packtime.  The storage can't do a full
+        # traversal as of the packtime, because the previous pack may
+        # have removed revisions necessary for a full traversal.
+
+        # It should be simple to test that a storage error is raised,
+        # but this test case goes to the trouble of constructing a
+        # scenario that would lose data if the earlier packtime was
+        # honored.
+
+        self._initroot()
+
+        db = DB(self._storage)
+        conn = db.open()
+        root = conn.root()
+
+        root["d"] = d = PersistentMapping()
+        get_transaction().commit()
+        snooze()
+
+        obj = d["obj"] = C()
+        obj.value = 1
+        get_transaction().commit()
+        snooze()
+        packt1 = time.time()
+        lost_oid = obj._p_oid
+
+        obj = d["anotherobj"] = C()
+        obj.value = 2
+        get_transaction().commit()
+        snooze()
+        packt2 = time.time()
+
+        db.pack(packt2)
+        # BDBStorage allows the second pack, but doesn't lose data.
+        try:
+            db.pack(packt1)
+        except StorageError:
+            pass
+        # This object would be removed by the second pack, even though
+        # it is reachable.
+        self._storage.load(lost_oid, "")
+
     def checkPackUndoLog(self):
         self._initroot()
         # Create a `persistent' object