Code review comment for lp:~gz/bzr/add_two_in_unicode_cwd_686611

Revision history for this message
Martin Packman (gz) wrote :

Thanks for the review Martin.

> > * ntpath.abspath returns os.getcwd (not u) with a boolean False path, until
> Python 2.6.5
>
> What does "with a boolean False path" mean here? Oh, I guess it means '', and
> then it returns os.getcwd(); that makes sense.

Yes, couldn't think of better wording there.

> I think the fix and the argument for it is pretty reasonable. I would suggest
> putting a comment summarizing it (or just pointing to this page) next to the
> osutils.has_symlinks() line.
>
> On reflection the test there should be if platform=='win32' if we're trying to
> work around a specifically Windows bug?

Well, this is where things are confusing. The underlying bug is in both posixpath and ntpath, but the osutils path functions for the posix case happen to avoid it by using str.decode rather than just casting the input to unicode. So, it's worth testing on all platforms.

The code used not to resolve symlinks, and continuing to not do that work if symlink support is absent helpfully avoids the bugginess. I'll add a comment to this effect.

> So my votes is to merge it with news, and perhaps with a comment on the if
> statement.

Will add NEWS, and file a follow up bug for actually fixing the osutils side of this problem.

« Back to merge proposal