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.

- 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.

- new_file = open(new_abs_path, 'r')
+ new_file = open(new_abs_path, 'rb')

Why is this changed?