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

John A Meinel wrote:
[...]
> 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?

Yeah, I think so. Also note that _run_cleanup_reporting_errors unconditionally
warns rather than checking for -Dcleanup, but I don't think that's particularly
valuable.

> 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))

The func passed to do_with_cleanups here is expected to mutate self.cleanups
(via calling add_cleanup). So passing in reversed(self.cleanups) here would
mean no cleanups get run!

I'd be happy to make it a deque for clarity, but as the longest cleanup chain we
have so far is 4 elements I didn't think it really mattered much in terms of
performance :)

> 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):

Hmm. There's the competing concern that when calling do_with_cleanups directly
with a pre-built literal list I think it may be clearer to have the cleanups
forwards, i.e.:

    do_with_cleanup(foo, [first_cleanup, second_cleanup])

But you're right that it might be a net win to bias towards the other way. I'll
try it out and see what I think.

> 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.

Ok, if you like. It held off doing that simply on the grounds that I hadn't
had use for it. It's easy to add.

> 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.

I don't follow... what signature are you proposing do_with_cleanups should have
instead? You need some way to keep the cleanups list separate from the *args
and **kwargs, but that means either putting the cleanups list as the first arg,
or as a positional arg between the function and its *args. Both options feel
ugly to me.

> 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.

Yeah.

> 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.

Thanks for doing so :)

-Andrew.