Merge lp:~lifeless/bzr/fix-terminal-spew into lp:bzr

It is a little bit odd to have an __exit__ that ignores the exception info passed to it, in the same way that "except: pass" tends to look a bit odd. As you say on IRC, "until we'd behave differently in an __exit__ call" then there's nothing much to do here. Certainly for the 'bzr' front-end we'd expect exceptions to be caught at this point... although if someone did use this in a 'with' statement in 2.5 it would stop e.g. KeyboardInterrupt propagating. So if the exc_* args are not None, I think BzrLibraryState.__exit__ should "return False" to tell 'with' statements to propagate the exception. And pass these arguments to the UI factory's __exit__, obviously... hmm, maybe should return it's true/false value?

I don't think that needs to be fixed before this can land — it's not a regression, and an overall improvement. But there is a small trap lurking here for advanced API users, so if it's not fixed then an XXX would be warranted.

The only other issue is documentation: bzrlib.initialize's docstring needs to describe its return value, and BzrLibraryState's docstring probably ought to mention that it implements the context manager introduced in Python 2.5. Either that, or you should add docstrings to __enter__ or __exit__, but I know which one is easier ;) It's a bit pedantic, but this is now a front door to the bzrlib API so it should be documented thoroughly and clearly.

Finally, I don't think you've addressed Martin[gz]'s concern about moving the UI cleanup earlier: in particular, if the call to sys.exitfunc() (i.e. to run the atexit hooks) causes something to use the UI, what happens? I think some of the -D flags that output end-of-run summaries (e.g. of HPSS call counts, or transport activity), might cause trouble here. Or maybe not, but I'd some reassurance about it :)

Re return codes - I'll make it return False everywhere; I would have thought that None == False ?

I'll document it too.

I was going to say that with this change we don't use atexit, but thats a lie.

We use it in medium.py, and I think thats actually a little unsafe. Concretely its _DebugCounter, and it registers atexit so that when gc hasn't happened, when atexit triggers, it can report.

That medium in particular will go boom if its atexit code triggers, because it exists to call into trace.note. (And thats not safe either, because trace.note can change when tests change, so this particular code is very unsafe for unittests - debug counts from one test can report into a different test or even the global ui).

I propose instead that the library state have a cleanup stack (\o/), that we make the library state also get stored to a global by initialise (stopgaps R us), that the test suite set an appropriate library state as part of its log file shenanigans and finally that DebugCounter register with the cleanup stack of the librarystate that it finds.

Further to this:
 - I'm going to cleanup the test log support stuff; it all fits in neatly.

Exception propogation return values - argh. Seems like cleanups :). If there are N contexts, which ones should we honour the return value of; should we OR/NOT/XOR/AND them?

sent to pqm by email