Refactored FileStorage transactional undo

As part of a project to provide object-level commit locks for ZEO, I'm
refactiring FileStorage to maintain transaction-specific data in
Tranaction.data.  This involved undo.  In trying to figure this out, I
found:

- A bug in _undoDataInfo, which I verified with some tests and

- _transactionalUndoRecord was maddeningly difficult to reason about
  (and thus change).

I was concerned less by the bug than my inability to know whether a
change to the code would be correct.

So I refactored the code, mainly transactionalUndoRecord, to make the
code easier to understand, fixing some logic errors (I'm pretty sure)
along the way.  This included lots of comments. (Comments are much
easier to compose when you're working out logic you didn't
understand.)

In addition to makeing the code cleaner, it allows undo to be handled
in cases that weren't handled before.

src/ZODB/FileStorage/FileStorage.py

@@ -788,13 +788,15 @@ def _undoDataInfo(self, oid, pos, tpos):
         """Return the tid, data pointer, and data for the oid record at pos
         """
         if tpos:
-            pos = tpos - self._pos - self._thl
+            itpos = tpos - self._pos - self._thl
+            pos = tpos
             tpos = self._tfile.tell()
-            h = self._tfmt._read_data_header(pos, oid)
+            h = self._tfmt._read_data_header(itpos, oid)
             afile = self._tfile
         else:
             h = self._read_data_header(pos, oid)
             afile = self._file
+
         if h.oid != oid:
             raise UndoError("Invalid undo transaction id", oid)
 
@@ -831,57 +833,83 @@ def _transactionalUndoRecord(self, oid, pos, tid, pre):
         pointer 0.
         """
 
-        copy = 1 # Can we just copy a data pointer
+        copy = True # Can we just copy a data pointer
 
         # First check if it is possible to undo this record.
         tpos = self._tindex.get(oid, 0)
         ipos = self._index.get(oid, 0)
         tipos = tpos or ipos
 
         if tipos != pos:
-            # Eek, a later transaction modified the data, but,
-            # maybe it is pointing at the same data we are.
-            ctid, cdataptr, cdata = self._undoDataInfo(oid, ipos, tpos)
+            # The transaction being undone isn't current because:
+            # a) A later transaction was committed ipos != pos, or
+            # b) A change was made in the current transaction. This
+            #    could only be a previous undo in a multi-undo.
+            #    (We don't allow multiple data managers with the same
+            #    storage to participate in the same transaction.)
+            assert tipos > pos
+
+            # Get current data, as identified by tipos.  We'll use
+            # it to decide if and how we can undo in this case.
+            ctid, cdataptr, current_data = self._undoDataInfo(oid, ipos, tpos)
 
             if cdataptr != pos:
-                # We aren't sure if we are talking about the same data
+
+                # if cdataptr was == pos, then we'd be cool, because
+                # we're dealing with the same data.
+
+                # Because they aren't equal, we have to dig deeper
+
+                # Let's see if data to be undone and current data
+                # are the same. If not, we'll have to decide whether
+                # we should try conflict resolution.
+
                 try:
-                    if (
-                        # The current record wrote a new pickle
-                        cdataptr == tipos
-                        or
-                        # Backpointers are different
-                        self._loadBackPOS(oid, pos) !=
-                        self._loadBackPOS(oid, cdataptr)
-                        ):
-                        if pre and not tpos:
-                            copy = 0 # we'll try to do conflict resolution
-                        else:
-                            # We bail if:
-                            # - We don't have a previous record, which should
-                            #   be impossible.
-                            raise UndoError("no previous record", oid)
+                    data_to_be_undone = self._loadBack_impl(oid, pos)[0]
+
+                    # Note that we use ``cdata or`` below, because
+                    # _loadBackPOS dosn't work with in-tranaaction
+                    # data and because we don't need to bother if we
+                    # already have the data.
+                    if not current_data:
+                        current_data = self._loadBack_impl(oid, cdataptr)[0]
+
+                    if data_to_be_undone != current_data:
+                        # OK, so the current data is different from
+                        # the data being undone.  We can't just copy:
+                        copy = False
+
+                        if not pre:
+                            # The transaction we're undoing has no
+                            # previous state to merge with, so we
+                            # can't resolve a conflict.
+                            raise UndoError(
+                                "Can't undo an add transaction followed by"
+                                " conflicting transactions.", oid)
                 except KeyError:
                     # LoadBack gave us a key error. Bail.
                     raise UndoError("_loadBack() failed", oid)
 
         # Return the data that should be written in the undo record.
         if not pre:
-            # There is no previous revision, because the object creation
-            # is being undone.
+            # We're undoing object addition.  We're doing this because
+            # subsequent transactions has no net effect on the state
+            # (possibly because some of them were undos).
             return "", 0, ipos
 
         if copy:
             # we can just copy our previous-record pointer forward
             return "", pre, ipos
 
         try:
-            bdata = self._loadBack_impl(oid, pre)[0]
+            pre_data = self._loadBack_impl(oid, pre)[0]
         except KeyError:
             # couldn't find oid; what's the real explanation for this?
             raise UndoError("_loadBack() failed for %s", oid)
+
         try:
-            data = self.tryToResolveConflict(oid, ctid, tid, bdata, cdata)
+            data = self.tryToResolveConflict(
+                oid, ctid, tid, pre_data, current_data)
             return data, 0, ipos
         except ConflictError:
             pass
@@ -1003,7 +1031,8 @@ def _txn_undo_write(self, tpos):
                             # We're undoing a blob modification operation.
                             # We have to copy the blob data
                             tmp = mktemp(dir=self.fshelper.temp_dir)
-                            with self.openCommittedBlobFile(h.oid, userial) as sfp:
+                            with self.openCommittedBlobFile(
+                                h.oid, userial) as sfp:
                                 with open(tmp, 'wb') as dfp:
                                     cp(sfp, dfp)
                             self._blob_storeblob(h.oid, self._tid, tmp)
@@ -1238,7 +1267,8 @@ def handle_file(path):
                     continue
 
                 if len(line) != 16:
-                    raise ValueError("Bad record in ", self.blob_dir, '.removed')
+                    raise ValueError(
+                        "Bad record in ", self.blob_dir, '.removed')
 
                 oid, tid = line[:8], line[8:]
                 path = fshelper.getBlobFilename(oid, tid)

src/ZODB/tests/TransactionalUndoStorage.py

@@ -800,3 +800,25 @@ def checkIndicesInUndoInfo(self):
 
     def checkIndicesInUndoLog(self):
         self._exercise_info_indices("undoLog")
+
+    def checkUndoMultipleConflictResolution(self, reverse=False):
+        from .ConflictResolution import PCounter
+        db = DB(self._storage)
+
+        with db.transaction() as conn:
+            conn.root.x = PCounter()
+
+        for i in range(4):
+            with db.transaction() as conn:
+                conn.transaction_manager.get().note(str(i))
+                conn.root.x.inc()
+
+        ids = [l['id'] for l in db.undoLog(1, 3)]
+        if reverse:
+            ids = list(reversed(ids))
+
+        db.undoMultiple(ids)
+        transaction.commit()
+
+    def checkUndoMultipleConflictResolutionReversed(self):
+        self.checkUndoMultipleConflictResolution(True)