Code review comment for lp://staging/~spiv/bzr/robust-cleanup-in-commit

59 +def _run_cleanup(func, *args, **kwargs):
60 + """Run func(*args, **kwargs), logging but not propagating any error it
61 + raises.
62 +
63 + :returns: True if func raised no errors, else False.
64 + """
65 + try:
66 + func(*args, **kwargs)
67 + except KeyboardInterrupt:
68 + raise
69 + except Exception, exc:
70 + _log_cleanup_error(exc)
71 + return False
72 + return True
73 +
74 +
75 +def _run_cleanup_reporting_errors(func, *args, **kwargs):
76 + try:
77 + func(*args, **kwargs)
78 + except KeyboardInterrupt:
79 + raise
80 + except Exception, exc:
81 + trace.mutter('Cleanup failed:')
82 + trace.log_exception_quietly()
83 + trace.warning('Cleanup failed: %s', exc)
84 + return False
85 + return True

Is it just me, or is "_run_cleanup_reporting_errors" doing exactly the same
thing as _run_cleanup, only it isn't calling out to a helper to log the error.
As such, I'm thinking one of them should be removed. Which should also simplify
the _run_cleanups() function.

I assume at one time one of them might have propagated the exception?

124 + def add_cleanup(self, cleanup_func):
125 + """Add a cleanup to run. Cleanups will be executed in LIFO order."""
126 + self.cleanups.insert(0, cleanup_func)

^- insert(0,) has to move all items in the list each time. Why not just do:

self.cleanups.append(cleanup_func)

and then

128 + def run(self, *args, **kwargs):
129 + func = lambda: self.func(self, *args, **kwargs)
130 + return do_with_cleanups(func, reversed(self.cleanups))

Or alternatively, I think the standard idiom would be to build up state in
forward order and then walk it reversed. so I would think it reasonable to
change '_run_cleanups' from:
    for func in funcs:
to
    for func in reversed(funcs):

I'm pretty sure you always want to run cleanups in LIFO order, so it would make
sense to bias the apis to do that easily.

Also, if we are going to have an "add_cleanup()" helper, it seems like it would
be good to make it the twin of TestCase.addCleanup(). Most notably, that api
(now) allows you to pass in arguments to the cleanup functions.

I also think that would make for a cleaner api for 'do_with_cleanups', since
otherwise you have this:
func = lambda: self.func(self, *args, **kwargs)

which works, but means that the api isn't very nice to use.

695 +#class TestRunCleanupReportingErrors(CleanupsTestCase):
696 +#
697 +# def test_cleanup_error_reported(self):
698 +# xxx

^- I'm assuming you just want to nuke this when you nuke
_run_cleanup_reporting_exceptions.

I do think commit.py is nicer with an object managing the state of things that
needs to be cleaned up. And I'm flexible about api decisions. I figured I could
state my opinion, though.