Break up transactionalUndo() into possibly understandable chunks.

transactionalUndo() does argument checking and locking.  It calls
_transactional_undo(), which finds the right transaction record.  It calls
_txn_undo_write(), which writes the data records.

Add a summary comment above undoLog() that explains how the
transaction_id is created and used.

src/ZODB/FileStorage.py

@@ -115,7 +115,7 @@
 #   may have a back pointer to a version record or to a non-version
 #   record.
 #
-__version__='$Revision: 1.87 $'[11:-2]
+__version__='$Revision: 1.88 $'[11:-2]
 
 import struct, time, os, string, base64, sys
 from struct import pack, unpack
@@ -971,7 +971,6 @@ class FileStorage(BaseStorage.BaseStorage,
         self._file.seek(pos+8)
         return self._file.read(8)
 
-
     def _transactionalUndoRecord(self, oid, pos, serial, pre, version):
         """Get the indo information for a data record
 
@@ -979,7 +978,6 @@ class FileStorage(BaseStorage.BaseStorage,
         version, packed non-version data pointer, and current
         position.  If the pickle is true, then the data pointer must
         be 0, but the pickle can be empty *and* the pointer 0.
-
         """
         
         copy=1 # Can we just copy a data pointer
@@ -1035,6 +1033,32 @@ class FileStorage(BaseStorage.BaseStorage,
 
         raise UndoError('Some data were modified by a later transaction')
 
+    # undoLog() returns a description dict that includes an id entry.
+    # The id is opaque to the client, but encodes information that
+    # uniquely identifies a transaction in the storage.  The id is a
+    # base64 encoded string, where the components of the string are:
+    #     - the transaction id
+    #     - the packed file position of the transaction record
+    #     - the oid of an object modified by the transaction
+
+    # The file position is sufficient in most cases, but doesn't work
+    # if the id is used after a pack and may not work if used with
+    # replicated storages.  If the file position is incorrect, the oid
+    # can be used for a relatively efficient search for the
+    # transaction record.  FileStorage keeps an index mapping oids to
+    # file positions, but do notes have a transaction id to file
+    # offset index.  The oid index maps to the most recent revision of
+    # the object.  Transactional undo must follow back pointers until
+    # it finds the correct transaction record,
+
+    # This approach fails if the transaction record has no data
+    # records.  It's not clear if that is possible, but it may be for
+    # commitVersion and abortVersion.
+
+    # The file offset also supports non-transactional undo, which
+    # won't work after a pack and isn't supported by replicated
+    # storages.
+
     def undoLog(self, first=0, last=-20, filter=None):
         if last < 0:
             last = first - last + 1
@@ -1073,28 +1097,6 @@ class FileStorage(BaseStorage.BaseStorage,
                         e = loads(read(el))
                     except:
                         pass
-                # We now need an encoded id that isn't dependent on file
-                # position, because it will break after a pack, and in the
-                # face of replication, while the transaction and data records
-                # may be identical (as viewed from the storage interface),
-                # file positions may be meaningless across replicas.
-                #
-                # We'd love to just give the tid, but FS makes it expensive to
-                # go from tid to transaction record. :(  However, if the txn
-                # has data records, then we can encode the oid of one of the
-                # objects affected by the txn.  Then we can use the index to
-                # find the current revision of the object, follow a
-                # back-pointer to find its most-current txn, and then follow
-                # the txns back until we find a match.  Seems like the best we
-                # can do w/o a persistent tid->filepos mapping.
-                #
-                # Note: if the txn has no data records, we're screwed.  Punt
-                # on that for now.
-                #
-                # Note that we're still encoding the transaction position
-                # in the transaction ID in order to support non-transactional
-                # undo.  This can be removed as soon as non-transactional
-                # undo is removed.
                 next = read(8)
                 # next is either the redundant txn length - 8, or an oid
                 if next == tl:
@@ -1134,6 +1136,11 @@ class FileStorage(BaseStorage.BaseStorage,
         
         self._lock_acquire()
         try:
+            return self._transactional_undo(transaction_id)
+        finally:
+            self._lock_release()
+
+    def _transactional_undo(self, transaction_id):
         # As seen in undoLog() below, transaction_id encodes the tid and
         # possibly the oid of the first object in the transaction record.
         # transaction_id will be of length 16 if there were objects
@@ -1143,6 +1150,8 @@ class FileStorage(BaseStorage.BaseStorage,
         # we're punting on that for now.
         transaction_id = base64.decodestring(transaction_id + '\n')
         tid = transaction_id[:8]
+        pos = U64(transaction_id[8:16])
+        # XXX
         oid = transaction_id[16:]
         if oid == '' or not self._index.has_key(oid):
             # We can't get the position of the transaction easily.
@@ -1159,7 +1168,8 @@ class FileStorage(BaseStorage.BaseStorage,
         while 1:
             self._file.seek(pos)
             h = self._file.read(DATA_HDR_LEN)
-                doid,serial,prev,tpos,vlen,plen = unpack('>8s8s8s8sH8s', h)
+            doid, serial, prev, tpos, vlen, plen = \
+                  unpack('>8s8s8s8sH8s', h)
             tpos = U64(tpos)
             self._file.seek(tpos)
             # Read transaction id to see if we've got a match
@@ -1170,17 +1180,24 @@ class FileStorage(BaseStorage.BaseStorage,
             pos = U64(prev)
             if not pos:
                 # We never found the right transaction
-                    raise UndoError, 'Invalid undo transaction id'
+                raise UndoError('Invalid undo transaction id')
         # We're sitting at the transaction we want to undo, but let's move
         # the file pointer back to the start of the txn record.
+        tindex = self._txn_undo_write(tpos, tid, ostloc, here)
+        self._tindex.update(tindex)
+        return tindex.keys()            
+
+    def _txn_undo_write(self, tpos, tid, ostloc, here):
+        # a helper function to write the data records for transactional undo
         self._file.seek(tpos)
         h = self._file.read(TRANS_HDR_LEN)
+        # XXX jer: don't think the second test is needed at this point
         if len(h) != TRANS_HDR_LEN or h[:8] != tid:
-                raise UndoError, 'Invalid undo transaction id'
+            raise UndoError('Invalid undo transaction id')
         if h[16] == 'u':
             return
         if h[16] != ' ':
-                raise UndoError, 'non-undoable transaction'
+            raise UndoError('non-undoable transaction')
         tl = U64(h[8:16])
         ul, dl, el = struct.unpack(">HHH", h[17:TRANS_HDR_LEN])
         tend = tpos + tl
@@ -1220,7 +1237,7 @@ class FileStorage(BaseStorage.BaseStorage,
                 if v:
                     vprev=self._tvindex.get(v, 0) or self._vindex.get(v, 0)
                     self._tfile.write(snv + p64(vprev) + v)
-                        self._tvindex[v]=here
+                    self._tvindex[v] = here
                     odlen = DATA_VERSION_HDR_LEN + len(v)+(plen or 8)
                 else:
                     odlen = DATA_HDR_LEN+(plen or 8)
@@ -1229,18 +1246,18 @@ class FileStorage(BaseStorage.BaseStorage,
                     self._tfile.write(p)
                 else:
                     self._tfile.write(p64(prev))
-                    tindex[oid]=here
-                    here=here+odlen
+                tindex[oid] = here
+                here += odlen
 
             pos=pos+dlen
             if pos > tend:
                 raise UndoError, 'non-undoable transaction'
 
-            if failures: raise UndoError(failures)
-            self._tindex.update(tindex)
-            return tindex.keys()            
+        if failures:
+            raise UndoError(failures)
+
+        return tindex
         
-        finally: self._lock_release()
 
     def versionEmpty(self, version):
         if not version: