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

Revision history for this message
methane (songofacandy) 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(
> + "?", "_"))
>
> The new encoding-then-decoding seems redundant, as do the changes in
> `_get_command`, what prompted you to add that when the last version was so
> simple?

Multibyte encoding may contains '?' as a trailing char.
So ``full_path.encode('mbcs', 'replace').replace('?', '_')`` may break
multibyte char.

And I hate self._root or prefix is changed through encode().decode() and bzr
attempts to write outside of _root.
So I do:
1) Write temporary file with safe unicode path.
2) Launch external diff with encoded path.

« Back to merge proposal