Break up the logic into a bunch of helper methods:
 _commit_objects()
 _commit_subtrans()
 _finish_one()
 _finish_rest()
and possibly
 _commit_error()

As a result of the changes, the high-level logic of a commit fits into
28 lines inside a try/finally.  There are lots of details hidden in
the methods, but the names capture the high-level behavior of the
helpers.

This fixes the transient failure in testExceptionInSubAbortSub() and
is presumed to be correct, too.  The old code had the comment:
    # This should never fail
but did not to guarantee that it never failed.  I'm assuming that the
write fix is to log the error and ignore it, because that's what the
call to tpc_abort() immediately above does.

XXX commit() is a really long method.  It would be nice to break it up
into easier-to-understand chunks.

including some use of assert/fail methods instead of assert stmt.

    The rationale is that this is an internal detail of the cache
    implementation called by Per_dealloc().  It's nice to make it
    fast, and it's good to prevent it from being called with anything
    other than an about-to-be-freed object.

    As a result of it being in the C API, omit tests of refcounts.  We
    would have been in Per_dealloc() otherwise.

    Initialize the percachdel slot of cPersistenceAPIstruct to NULL in
    cPersistence.  In cPickleCache, import the CObject and fill in the
    struct.

Also, reformat many of the comments so that all the text starts to the
right of the /*.

Fiddle the text of the placeholder comment.  I think I understand it
now :-).

Add an XXX comment about the "old" cache API e.g. fullsweep and
reallyfullsweep.

Fix cc_get().  object_from_oid() doesn't ever set an exception.

Remove a Py_FatalError() check from the end of a module init
function.  We don't do that anymore.

I expect this code will become ZEO 1.1.

description fields, which are assigned to attributes on the
Transaction object.

takes an exception object as its first argument, and the return value
should be a -1 instead of NULL (this function's signature has an int
return).

I'm surprised no one else's build caught this compiler warning.

Merged shane-better-tracebacks-branch.  The changes are explained in http://dev.zope.org/Wikis/DevSite/Proposals/BetterTracebacks

(Thanks for the comments, Toby!)

Add check that argument to cc_ass_sub() is a string.  The cache maps
oids to objects, and oids must be strings for now.

Re-indent a bunch of code.

Replace an XXX comment about v->oid with a comment explaining why
getattr is necessary.

on ->cache and references to its members.

Quite a bit of reformatting and unless-undoing.  Add whitespace
between 'if' and '('.

ring_home and use it explicitly.

Remove now unused KEEP_THIS_ONE_AROUND_FOR_A_WHILE() macro.

check_ring() rename to ring_corrupt() since that matches the return
semantics a bit better.

scan_gc_items(): Get rid of the placeholder, which just seemed like a
confusing way to stash here->next until after the PyObject_SetAttr()
call.

UPDATE_STATE_IF_NECESSARY: Un-macroize this.  Renamed to function
unghostify() and changed (slightly) the return semantics.

ghostify(): Don't use the NON_GHOST_COUNT macro.  Add some comments.

Per_setstate(), Per__getstate__(), Per_getattr(), _setattro(): Use
unghostify() instead of UPDATE_STATE_IF_NECESSARY.

did you use?

A few substantive changes:

Split internals of cc_ass_sub() into two smaller function
cc_add_item() and cc_del_item(), removing several levels of
indentation in the process.

Fix incorrect check for return value from PyDict_SetItem().

Remove use of staticforward.

Avoid forward reference to Cctype by defining newccobject after type
struct.

Define the contents of ccobject_head_struct via CACHE_HEAD macro.
Don't rely on two different files to define a struct with the same
layout by convention.

Remove many levels of parentheses from the HOME and NON_GHOST_COUNT
macros.

Replace KEEP_THIS_ONE_AROUND_FOR_A_WHILE() macro with a call to the
accessed() function.  (Who comes up with these names :-).

Remove include of string.h in cPersistence.c.

Expand cPersistent_HEAD macro so that it has one line for each
attribute!

Use typedef to define PerCache as "struct ccobject_head_struct" and
use this for the entry cache slot in cPersistent_HEAD instead of
PyObject *.

In cPickleCache.c, reflow a bunch of long lines, add some whitespace,
and an XXX comment explaining that I don't understand a different
comment.

These tests verify that transactional undo works with abortVersion and
commitVersion, even in the face of a pack that invalidates the file
position contained in the transaction record.

Use the file position stored in the transaction_id whenever possible.
It is possible when _check_txn_pos() returns true; i.e. when the file
position does point to the transaction record header.

Remove ostloc and here arguments to _txn_undo_write(), as they can be
computed after the call.

Omit redundant tests in _txn_undo_write().  All the paths that lead
here verify that the file position is valid.

Remove some attribute lookup optimizations.

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.

It makes more sense in this order, because the comments in undoLog()
help to explain what transactionalUndo() is doing.

appears to be broken.

undo to continue to work, and the IDs have to be base64 encoded to work on
HTML forms.  In future, non-transactional undo may be removed and the HTML
forms might be updated to perform the encoding of transaction IDs.

that environment, it is possible that all the data in the replicas
recoverable through the public api are identical, while the actual
on-disc representation differs.  This bug can be boiled down to doing
the following in "standard" ZODB:

    1. undoLog()
    2. pack()
    3. transactionalUndo()

Previously, undoLog() encoded the file position in the 'id' key of the
undo record, but of course the pack() breaks that file position.  We
fix this by encoding the oid of an object touched in the transaction
instead.  That way, transactionalUndo() can start at the current
revision of the object, and scan back until it can find a transaction
with a matching id to the one we're undoing.

This approach breaks when the transaction we're undoing doesn't touch
any objects, e.g. is an abortVersion() or commitVersion().  By edict,
that's a non-requirement for now.  We could fix that by doing a more
expensive binary search for the matching transaction.

WIBNI we had an index from tids to file positions? :)

really testing what we're interested in (they were testing the
equality of the exact layout of the undoInfo() -- not interesting).

suspicion that FileStorage (and an FS-backed ZEO) fails a
transactional undo after a pack, but Berkeley storage passes.

Two problems: A revid doesn't have the right type.  But even if we
decoded the revid using TimeStamp(revid).timeTime(), it would be the
wrong time.  We want to pack to the current time to delete older
revisions.

I believe the test case tried to keep one revision available so that
it could be packed.  The new test doesn't do that, so it will fail
even after we fix FileStorage.  I think the right answer is to call
time.time() after the second commit *and* make sure that there is at
least a one second delay between the two stores.

the underlying bug in transactionalUndo() on a standby storage.  If
our hypothesis is correct, the bug is in FileStorage, and is caused by
encoding the file position in the `id' field of the undoLog
information.  Note that Full just encodes the tid, but this is a
problem for FileStorage (we have a strategy for fixing this).

NOTE: checking this in on the trunk is mildly antisocial because it
introduces failures in the test suite.  I'm doing it this way because
1) it really isn't worth a branch, even a short lived one; 2) Jeremy
may have time to look at it before tomorrow; 3) I don't trust my
machine not to lock up and make this mod unavailable. :(

Either way, this /will/ get fixed by tomorrow.

The --nowrite option will disable writing.  (It's not possible to
enable writing and disable reading, so that's the only option.)

The problem was that the randomly generated test data was occasionally
invalid.  Fix is to hardcode a specific set of test data instead of
using random.