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

Seems nice to me.

Looks reasonable to me.

The only problem I see is that bzrlib.clean_up now assumes that bzrlib.ui.ui_factory has a __exit__ method. I expect that's generally true for /usr/bin/bzr, but it might not be true for other existing clients of bzrlib (including loggerhead, perhaps?). So I think you should 1) document this new requirement of UIFactory objects in NEWS as an API Change, and 2) add empty implementations to the UIFactory base class.

(I'm voting "Needs Fixing" in the bb:tweak sense.)

IIRC, ContextManager is for Python 2.5 and later?

Alexander Belchenko wrote:
> IIRC, ContextManager is for Python 2.5 and later?

The 'with' statement is for Python 2.5 or later. There's nothing that
prevents us from creating objects with __enter__ and __exit__ methods in
Python 2.4. Obviously Python 2.4 can't use those objects in 'with'
statements, but it can call the methods directly as Aaron does in this
patch.

The _flush_stdout_stderr and _flush_trace functions in bzrlib.trace both have comments at the top stating they'll be run from atexit, should be changed if it's going to no longer be the case.

This change effectively moves the ui teardown to an earlier point, will that cause issues with messages being printed on object destruction?

Martin[gz] - side note, if you find something that should be changed, please do vote 'needs fixing' rather than a simple comment - the votes are usually instructive when looking at a proposal.

I actually have a deeper question - rather than having a library 'clean up' function, what about having bzrlib.initialize return something that can be used to clean up ? This would solve a bug in this proposal - calling cleanup() when initialize hasn't been called, or initialize then cleanup twice will fail because ui_factory will be None, so getting to the exit function is impossible :)

e.g.
if __name__ == '__main__':
- bzrlib.initialize()
+ clean_up = bzrlib.initialize()
- exit_val = bzrlib.commands.main()
+ try:
+ exit_val = bzrlib.commands.main()

- if profiling:
- profile_imports.log_stack_info(sys.stderr)
+ if profiling:
+ profile_imports.log_stack_info(sys.stderr)
+ finally:
+ clean_up()

I agree with the things that Martin[gz] noted too, so marking Needs Fixing. Aaron - totally up to you if you want to change it to be something returned from initialize: I think that might be better in the long run - taking us away from globals, but its up to you.

I'm polishing - see my fix-terminal-spew branch.