[MERGE] Restore environment variables in a timely fashion (bug 124153)

[Jonathan Lange]

On 7/9/07, Andrew Bennetts <andrew at canonical.com> wrote:
> Jonathan Lange wrote:
> [...]
> >      def overrideEnvironmentForTesting(self):
> > +        old_home = os.environ['HOME']
> > +        old_bzr_home = os.environ['BZR_HOME']
> > +        def _restore_environment():
> > +            os.environ['HOME'] = old_home
> > +            os.environ['BZR_HOME'] = old_bzr_home
> >          os.environ['HOME'] = self.test_home_dir
> >          os.environ['BZR_HOME'] = self.test_home_dir
> > +        self.addCleanup(_restore_environment)
>
> You should use bzrlib.osutils.set_or_unset_env here.  See how
> TestCase._cleanupEnvironment works for an example (and ideally perhaps re-use
> TestCase._captureVar?).
>

The second version I sent through uses set_or_unset_env.

Re-using _captureVar requires significant refactoring. The problem is
that TestCase._restoreEnvironment is called too late for
_finishLogFile.

> Also, isn't the root problem really that:
>
>     opaque_handle = enable_test_log(to_file)
>     disable_test_log(opaque_handle)
>
> isn't the no-op it should be?  Because it ends up effectively calling
> enable_default_logging() instead of leaving the logging system uninitialised?
>

Right. As I said on the ticket, bzrlib.trace's state is fragile. To
me, they are both separate problems.

> So I think perhaps a better fix would be to make enable/disable_test_log
> actually *just* restore the state of the trace module, and not call
> disable/enable_default_logging.
>

I agree that that would make a lot of sense, though I'd have to ask
someone else to make that change.

FWIW, I still think my patch should be merged. It's correct (modulo
set_or_unset_env issues), small and solves the immediate problem.

jml