undoInfo() and undoLog() almost always returned wrong # of results.

Repaired, + new tests.

NEWS.txt

@@ -6,6 +6,7 @@ What follows is combined news from the "internal releases" (to support
 ongoing Zope 2.8 and Zope3 development) since the last public ZODB 3.4
 release.  These are the dates of the internal releases:
 
+- 3.4a9 DD-MMM-2005
 - 3.4a8 09-May-2005
 - 3.4a7 06-May-2005
 - 3.4a6 05-May-2005
@@ -121,6 +122,18 @@ Tools
   This actually went in several months go, but wasn't noted here at the time.
   Thanks to Dmitry Vasiliev for contributing code and tests.
 
+FileStorage
+-----------
+
+- (3.4a9) The ``undoLog()`` and ``undoInfo()`` methods almost always returned
+  a wrong number of results, one too many if ``last < 0`` (the default is
+  such a case), or one too few if ``last >= 0``.  These have been repaired,
+  new tests were added, and these methods are now documented in
+  ``ZODB.interfaces.IStorageUndoable``.
+
+- (3.4a2) A ``pdb.set_trace()`` call was mistakenly left in method
+  ``FileStorage.modifiedInVersion()``.
+
 DemoStorage
 -----------
 
@@ -149,12 +162,6 @@ Tests
 - (3.4a2) The test ``checkOldStyleRoot`` failed in Zope3, because of an
   obscure dependence on the ``Persistence`` package (which Zope3 doesn't use).
 
-FileStorage
------------
-
-- (3.4a2) A ``pdb.set_trace()`` call was mistakenly left in method
-  ``FileStorage.modifiedInVersion()``.
-
 ZApplication
 ------------
 

src/ZODB/FileStorage/FileStorage.py

@@ -1069,7 +1069,12 @@ class FileStorage(BaseStorage.BaseStorage,
 
     def undoLog(self, first=0, last=-20, filter=None):
         if last < 0:
-            last = first - last + 1
+            # -last is supposed to be the max # of transactions.  Convert to
+            # a positive index.  Should have x - first + 1 = -last, which
+            # means x = first - last - 1.  This is spelled out here because
+            # the normalization code was incorrect for years (used +1
+            # instead -- off by 2), until ZODB 3.4.
+            last = first - last - 1
         self._lock_acquire()
         try:
             if self._pack_is_in_progress:
@@ -2036,14 +2041,18 @@ class UndoSearch:
         self.first = first
         self.last = last
         self.filter = filter
+        # self.i is the index of the transaction we're _going_ to find
+        # next.  When it reaches self.first, we should start appending
+        # to self.results.  When it reaches self.last + 1, we're done
+        # (although we may finish earlier).
         self.i = 0
         self.results = []
-        self.stop = 0
+        self.stop = False
 
     def finished(self):
         """Return True if UndoSearch has found enough records."""
         # BAW: Why 39 please?  This makes no sense (see also below).
-        return self.i >= self.last or self.pos < 39 or self.stop
+        return self.i > self.last or self.pos < 39 or self.stop
 
     def search(self):
         """Search for another record."""

src/ZODB/tests/TransactionalUndoStorage.py

@@ -724,3 +724,59 @@ class TransactionalUndoStorage:
         self.assertEqual(d['description'],'t1')
         self.assertEqual(d['k2'],'this is transaction metadata')
         self.assertEqual(d['user_name'],'p3 u3')
+
+    # A common test body for index tests on undoInfo and undoLog.  Before
+    # ZODB 3.4, they always returned a wrong number of results (one too
+    # few _or_ too many, depending on how they were called).
+    def _exercise_info_indices(self, method_name):
+        db = DB(self._storage)
+        info_func = getattr(db, method_name)
+        cn = db.open()
+        rt = cn.root()
+
+        # Do some transactions.
+        for key in "abcdefghijklmnopqrstuvwxyz":
+            rt[key] = ord(key)
+            transaction.commit()
+
+        # 26 letters = 26 transactions, + the hidden transaction to make
+        # the root object, == 27 expected.
+        allofem = info_func(0, 100000)
+        self.assertEqual(len(allofem), 27)
+
+        # Asking for no more than 100000 should do the same.
+        redundant = info_func(last=-1000000)
+        self.assertEqual(allofem, redundant)
+
+        # By default, we should get only 20 back.
+        default = info_func()
+        self.assertEqual(len(default), 20)
+        # And they should be the most recent 20.
+        self.assertEqual(default, allofem[:20])
+
+        # If we ask for only one, we should get only the most recent.
+        fresh = info_func(last=0)
+        self.assertEqual(len(fresh), 1)
+        self.assertEqual(fresh[0], allofem[0])
+
+        # Another way of asking for only the most recent.
+        redundant = info_func(last=-1)
+        self.assertEqual(fresh, redundant)
+
+        # Try a slice that doesn't start at 0.
+        oddball = info_func(first=11, last=17)
+        self.assertEqual(len(oddball), 17-11+1)
+        self.assertEqual(oddball, allofem[11 : 11+len(oddball)])
+
+        # And another way to spell the same thing.
+        redundant = info_func(first=11, last=-7)
+        self.assertEqual(oddball, redundant)
+
+        cn.close()
+        db.close()
+
+    def checkIndicesInUndoInfo(self):
+        self._exercise_info_indices("undoInfo")
+
+    def checkIndicesInUndoLog(self):
+        self._exercise_info_indices("undoLog")