Merge rev 33273 from 3.4 branch.

Merge tim-savepoint branch.

Allow rollback to a given savepoint multiple times.

transaction/_transaction.py
    _remove_and_invalidate_after():  Don't remove the
    argument savepoint from the savepoint stack anymore.

    abort():  abort(True) relied on the subtxn savepoint
    being invalidated as a side effect of rolling back
    to it.  Invalidate it explicitly now.

    Savepoint.rollback():  Don't invalidate self.

ZODB/Connection.py
    Connection._rollback():  Pass a temp container to
    _cache.invalidate(), because that method clears the
    container as a side effect.  Clearing the index
    in a savepoint was a disaster if the savepoint was
    used again.

    TmpStore.reset():  Make a copy of the argument index.
    An alternative would be to make all callers make
    copies, but the only caller now thinks of its `state`
    argument as being opaque.

transaction/savepoint.txt
ZODB/tests/{testSubTransaction.py, testConnectionSavepoint.txt}
    Test the new semantics, and beef up the tests.

transaction/tests/savepointsample.py
    _rollback_savepoint():  Needed to copy the data.

NEWS.txt

@@ -21,6 +21,12 @@ Savepoints
   could cause spurious errors if a transaction with a pending savepoint
   needed to fetch an older revision of some object.
 
+- (3.5a4) The ``ISavepoint`` interface docs said you could roll back to a
+  given savepoint any number of times (until the transaction ends, or until
+  you roll back to an earlier savepoint's state), but the implementation
+  marked a savepoint as invalid after its first use.  The implementation has
+  been repaired, to match the docs.
+
 Multi-database
 --------------
 
@@ -80,6 +86,7 @@ Release date: DD-MMM-2005
 Following are dates of internal releases (to support ongoing Zope 2
 development) since ZODB 3.4's last public release:
 
+- 3.4.1a5 DD-MMM-2005
 - 3.4.1a4 08-Jul-2005
 - 3.4.1a3 02-Jul-2005
 - 3.4.1a2 29-Jun-2005
@@ -96,6 +103,12 @@ Savepoints
   could cause spurious errors if a transaction with a pending savepoint
   needed to fetch an older revision of some object.
 
+- (3.4.1a5) The ``ISavepoint`` interface docs said you could roll back to a
+  given savepoint any number of times (until the transaction ends, or until
+  you roll back to an earlier savepoint's state), but the implementation
+  marked a savepoint as invalid after its first use.  The implementation has
+  been repaired, to match the docs.
+
 FileStorage
 -----------
 

src/ZODB/Connection.py

@@ -1069,7 +1069,13 @@ class Connection(ExportImport, object):
         self._abort()
         self._registered_objects = []
         src = self._storage
-        self._cache.invalidate(src.index)
+        # Caution:  it's possible that src.index is part of `state`, and
+        # invalidate() clears the container passed to it.  When rollback() is
+        # called on the same savepoint more than once, that's even likely.
+        # Therefore we must pass a copy of the oids to invalidate, lest
+        # we next reset `src` to have an empty index:  the ".keys()" here
+        # is vital.
+        self._cache.invalidate(src.index.keys())
         src.reset(*state)
 
     def _commit_savepoint(self, transaction):
@@ -1193,4 +1199,13 @@ class TmpStore:
     def reset(self, position, index):
         self._file.truncate(position)
         self.position = position
-        self.index = index
+        # Caution:  We're typically called as part of a savepoint rollback.
+        # Other machinery remembers the index to restore, and passes it to
+        # us.  If we simply bind self.index to `index`, then if the caller
+        # didn't pass a copy of the index, the caller's index will mutate
+        # when self.index mutates.  This can be a disaster if the caller is a
+        # savepoint to which the user rolls back again later (the savepoint
+        # loses the original index it passed).  Therefore, to be safe, we make
+        # a copy of the index here.  An alternative would be to ensure that
+        # all callers pass copies.  As is, our callers do not make copies.
+        self.index = index.copy()

src/ZODB/tests/testConnectionSavepoint.txt

@@ -134,3 +134,59 @@ away:
 
     >>> root['sally-balance']
     0.0
+
+Savepoint invalidation
+----------------------
+
+A savepoint can be used any number of times:
+
+    >>> root['bob-balance'] = 100.0
+    >>> root['bob-balance']
+    100.0
+    >>> savepoint = transaction.savepoint()
+
+    >>> root['bob-balance'] = 200.0
+    >>> root['bob-balance']
+    200.0
+    >>> savepoint.rollback()
+    >>> root['bob-balance']
+    100.0
+
+    >>> savepoint.rollback()  # redundant, but should be harmless
+    >>> root['bob-balance']
+    100.0
+
+    >>> root['bob-balance'] = 300.0
+    >>> root['bob-balance']
+    300.0
+    >>> savepoint.rollback()
+    >>> root['bob-balance']
+    100.0
+
+However, using a savepoint invalidates any savepoints that come after it:
+
+    >>> root['bob-balance'] = 200.0
+    >>> root['bob-balance']
+    200.0
+    >>> savepoint1 = transaction.savepoint()
+
+    >>> root['bob-balance'] = 300.0
+    >>> root['bob-balance']
+    300.0
+    >>> savepoint2 = transaction.savepoint()
+
+    >>> savepoint.rollback()
+    >>> root['bob-balance']
+    100.0
+
+    >>> savepoint2.rollback()
+    Traceback (most recent call last):
+    ...
+    InvalidSavepointRollbackError
+
+    >>> savepoint1.rollback()
+    Traceback (most recent call last):
+    ...
+    InvalidSavepointRollbackError
+
+    >>> transaction.abort()

src/ZODB/tests/testSubTransaction.py

@@ -97,11 +97,12 @@ database state as of the last sub-transaction commit.  There is
 
 >>> c.value = "c1"
 >>> transaction.abort(1)
+>>> a.value, b.value, c.value
+('a1', 'b1', 'c0')
 
 Multiple aborts have no extra effect.
 
 >>> transaction.abort(1)
-
 >>> a.value, b.value, c.value
 ('a1', 'b1', 'c0')
 

src/transaction/_transaction.py

@@ -303,13 +303,13 @@ class Transaction(object):
 
         return savepoint
 
-    # Remove `savepoint` from _savepoint2index, and also remove and invalidate
-    # all savepoints we know about with an index larger than `savepoint`'s.
-    # This is what's needed when a rollback _to_ `savepoint` is done.
+    # Remove and invalidate all savepoints we know about with an index
+    # larger than `savepoint`'s.  This is what's needed when a rollback
+    # _to_ `savepoint` is done.
     def _remove_and_invalidate_after(self, savepoint):
         savepoint2index = self._savepoint2index
-        index = savepoint2index.pop(savepoint)
-        # use items to make copy to avoid mutating while iterating
+        index = savepoint2index[savepoint]
+        # use items() to make copy to avoid mutating while iterating
         for savepoint, i in savepoint2index.items():
             if i > index:
                 savepoint.transaction = None # invalidate
@@ -359,7 +359,6 @@ class Transaction(object):
         # transaction.
 
     def commit(self, subtransaction=False):
-
         if self._savepoint2index:
             self._invalidate_all_savepoints()
 
@@ -466,15 +465,17 @@ class Transaction(object):
                                rm, exc_info=sys.exc_info())
 
     def abort(self, subtransaction=False):
-
         if subtransaction:
-            # TODO deprecate subtransactions
+            # TODO deprecate subtransactions.
             if not self._subtransaction_savepoint:
                 raise interfaces.InvalidSavepointRollbackError
             if self._subtransaction_savepoint.valid:
-                # We're supposed to be able to call abort(1) multiple
-                # times. Sigh.
                 self._subtransaction_savepoint.rollback()
+                # We're supposed to be able to call abort(1) multiple
+                # times without additional effect, so mark the subtxn
+                # savepoint invalid now.
+                self._subtransaction_savepoint.transaction = None
+                assert not self._subtransaction_savepoint.valid
             return
 
         if self._savepoint2index:
@@ -661,14 +662,13 @@ class Savepoint:
         transaction = self.transaction
         if transaction is None:
             raise interfaces.InvalidSavepointRollbackError
-        self.transaction = None
         transaction._remove_and_invalidate_after(self)
 
         try:
             for savepoint in self._savepoints:
                 savepoint.rollback()
         except:
-            # Mark the transaction as failed
+            # Mark the transaction as failed.
             transaction._saveCommitishError() # reraises!
 
 class AbortSavepoint:

src/transaction/savepoint.txt

@@ -142,43 +142,57 @@ away:
 Savepoint invalidation
 ----------------------
 
-Once a savepoint has been used, it can't be used again:
+A savepoint can be used any number of times:
 
-    >>> savepoint = transaction.savepoint()
     >>> dm['bob-balance'] = 100.0
     >>> dm['bob-balance']
     100.0
+    >>> savepoint = transaction.savepoint()
 
-    >>> savepoint.rollback()
+    >>> dm['bob-balance'] = 200.0
     >>> dm['bob-balance']
-    0.0
-
+    200.0
     >>> savepoint.rollback()
-    Traceback (most recent call last):
-    ...
-    InvalidSavepointRollbackError
+    >>> dm['bob-balance']
+    100.0
 
-Using a savepoint also invalidates any savepoints that come after it:
+    >>> savepoint.rollback()  # redundant, but should be harmless
+    >>> dm['bob-balance']
+    100.0
 
-    >>> savepoint1 = transaction.savepoint()
-    >>> dm['bob-balance'] = 100.0
+    >>> dm['bob-balance'] = 300.0
+    >>> dm['bob-balance']
+    300.0
+    >>> savepoint.rollback()
     >>> dm['bob-balance']
     100.0
 
-    >>> savepoint2 = transaction.savepoint()
+However, using a savepoint invalidates any savepoints that come after it:
+
     >>> dm['bob-balance'] = 200.0
     >>> dm['bob-balance']
     200.0
+    >>> savepoint1 = transaction.savepoint()
 
-    >>> savepoint1.rollback()
+    >>> dm['bob-balance'] = 300.0
     >>> dm['bob-balance']
-    0.0
+    300.0
+    >>> savepoint2 = transaction.savepoint()
+
+    >>> savepoint.rollback()
+    >>> dm['bob-balance']
+    100.0
 
     >>> savepoint2.rollback()
     Traceback (most recent call last):
     ...
     InvalidSavepointRollbackError
 
+    >>> savepoint1.rollback()
+    Traceback (most recent call last):
+    ...
+    InvalidSavepointRollbackError
+
     >>> transaction.abort()
 
 Databases without savepoint support

src/transaction/tests/savepointsample.py

@@ -11,9 +11,10 @@
 # FOR A PARTICULAR PURPOSE.
 #
 ##############################################################################
-"""Savepoint data manager implementation example
+"""Savepoint data manager implementation example.
 
-Sample data manager implementation that illustrates how to implement savepoints
+Sample data manager implementation that illustrates how to implement
+savepoints.
 
 See savepoint.txt in the transaction package.
 
@@ -159,12 +160,17 @@ class SampleSavepointDataManager(SampleDataManager):
     interface.implements(transaction.interfaces.ISavepointDataManager)
 
     def savepoint(self):
-        # When we create the savepoint, we save the existing database state
+        # When we create the savepoint, we save the existing database state.
         return SampleSavepoint(self, self.uncommitted.copy())
 
     def _rollback_savepoint(self, savepoint):
-        # when we rollback the savepoint, we restore the saved data
-        self.uncommitted = savepoint.data
+        # When we rollback the savepoint, we restore the saved data.
+        # Caution:  without the copy(), further changes to the database
+        # could reflect in savepoint.data, and then `savepoint` would no
+        # longer contain the originally saved data, and so `savepoint`
+        # couldn't restore the original state if a rollback to this
+        # savepoint was done again.  IOW, copy() is necessary.
+        self.uncommitted = savepoint.data.copy()
 
 class SampleSavepoint: