Code review comment for lp:~songofacandy/bzr/user_encoding_utf8

Revision history for this message
Vincent Ladeuil (vila) wrote :

> That's really nice, thanks.
>
> It would be nice if this could also check a variable in the global
> configuration, so that people can set it permanently in bazaar.conf. Some
> people may find that easier than getting an environment variable set,
> especially on windows or on remote machines. But, we could come back and do
> that later.

Even better would be to use only a config variable since env variables are not universal as you point out. In this precise case there (use encoding) I can't think of a use case where an env variable will be more appropriate.

> Doing it from config may cause some bootstrapping issues, as bzr
> needs to know the encoding pretty early on, probably before it loads any
> configuration.

I don't think so, config files are encoded in utf-8 only and cannot be encoded into anything else.

63 + old_env = os.environ.get('BZR_USER_ENCODING')
64 + def cleanup():
65 + osutils.set_or_unset_env('BZR_USER_ENCODING', old_env)
66 + self.addCleanup(cleanup)

As mentioned by bialix, the idiom here should be:

   self.overrideEnv('BZR_USER_ENCODING', <new value>)

But since osutils.get_user cache its result, this won't work reliably you also need:

   self.overrideAttr(osutils, '_cached_user_encoding', None)

So anyway, better switch to config variables.

review: Needs Fixing

« Back to merge proposal