Code review comment for lp:~songofacandy/bzr/fix-523746-2

Revision history for this message
methane (songofacandy) wrote :

> Martin [gz] wrote:
> > Ack, was that big a follow up needed? I only intended to suggest something
> like:
> >
> > - full_path = osutils.pathjoin(self._root, prefix, relpath)
> > + full_path = osutils.pathjoin(self._root, prefix, relpath.encode(
> > + sys.getfilesystemencoding() or 'ascii', 'replace').replace(
> > + "?", "_"))
>
> ^- This should probably be 'osutils._fs_enc' where we already handle fs
> encoding of None.

subprocess.Popen uses os.execv(), and os.execv() use Py_FileSystemDefaultEncoding (=sys.getfilesystemencoding()) to encode arguments. When filesystemencoding is NULL, fallbacks to defaultencoding.

osutils._fs_enc fallbacks to 'utf-8'. So using osutils._fs_enc may cause UnicodeEncodeError on Popen.
# I think osutils._fs_enc fallbacks to utf-8 is good because I hope bzr uses
# utf-8 when I set LANG=C.

« Back to merge proposal