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

> This problem contains many situation.
> I think your fix doesn't work

That's why I wanted you to test it :) Did you ? If yes, did it fail and how ?

>sometimes because:
>
> 1. osutils.get_user_encoding() is not good in rare case.
> user_encoding is not equals to filesystem encoding.

My assumption was that subprocess.Popen should receive its arguments as if
they were coming from the command line: in user encoding.

>
> 2. path.encode(enc) can raise UnicodeEncodeError.
> So bzr should use "encode(enc, 'replace')"

That will mean some path can't be encoded in user encoding which in turn means
it could not be user as a parameter to bzr no ?

>
> 3. _write_file() writes file with unicode path.
> But if bzr can't encode the path collectly, external command can't
> open the file.

We rely on python to do the right thing from unicode paths there.

> So bzr should encode the path before _write_file().
>
> To test these situation:
>
> * Use fixed encoding in the test.

That's what the tests are doing for the user encoding.

> * Use filenames that contains chars that can't encode with the encoding.

Hmm, AFAIR, when we encounter problems there it's because some setting is wrong in the environment. As mentioned below, I couldn't automate the tests enough
to reach that point, hence my call for real-life tests :)

> * Replace subprocess.call and check arguments.

I don't want to do that, if the problem is in subprocess I will not be able to detect it.

That's the main point I dislike with the current tests, they don't exercise
subprocess.Popen, but I don't have an external diff tool I can call available
on all platforms.