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

> Vincent Ladeuil wrote on 2010-04-21 :
> > 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.
>
> I've fixed that in this branch: lp:~garyvdm/bzr/523746-difftool-file-names
>
> Those tests fail in wine for both Vincent's and INADA's branches.

I'll see the branch later.

> - new_abs_path = self._prepare_files(file_id, old_path, new_path,
> - allow_write_new=True,
> - force_temp=True)[1]
> - command = self._get_command(osutils.pathjoin('old', old_path),
> - osutils.pathjoin('new', new_path))
> + old_abs_path, new_abs_path = self._prepare_files(
> + file_id, old_path, new_path,
> + allow_write_new=True,
> + force_temp=True)
> + command = self._get_command(old_abs_path, new_abs_path)
>
> This is a nice simple change. Maybe we should merge this straight away,
> separately from the other changes.

I think so.

> - new_file = open(new_abs_path, 'r')
> + new_file = open(new_abs_path, 'rb')
>
> Why is this changed?

This change is made because I think bzr should use 'binary' mode in this point.
External diff tool may have edit feature and user may fix line ending. So bzr should keep
EOL bytes.

But, this change is not relating to #523746.