Merge lp:~gz/bzr/add_two_in_unicode_cwd_686611 into lp:bzr/2.0

[Martin Packman]

Avoid UnicodeDecodeError in ``bzr add` on windows

This fixes a regression from the fix for bug 192859 (hence the merge to 2.0) which breaks smart_add with certain unicode inputs, on windows only. Rather than launching on fixing all the windows path functions, this branch just gets bzr back to its previous state by making the problematic function call conditional on symlinks actually being supported.

This is is complex edge case involving multiple bugs, including an upstream issue† that has been fixed in recent Python versions, but the gist is:
* smart_add now calls the (undocumented, untested) normalizepath
* normalizepath uses osutils.realpath on the path dirname to resolve symlinks
* osutils.realpath on nix encodes to bytes and decodes from bytes so always returns unicode
* osutils.realpath on windows is basically ntpath.abspath
* ntpath.abspath returns os.getcwd (not u) with a boolean False path, until Python 2.6.5
* normalizepath tries to recombine that bytestring dirname with the unicode basename

Note, symlinks will never be supported on Python 2 on windows‡ so there's no future compatibility issue to worry about.

There's a whole bunch of extra weirdness here, like I have no idea why builtins.tree_files_for_add only makes the first path absolute, and with a different method.

I'm not sure what to do about NEWS, as this branch predates the system switch.

† os.path.abspath with unicode argument should call os.getcwdu
<http://bugs.python.org/issue3426>
‡ Add os.symlink() and os.path.islink() support for Windows
MvL: "The patch has now missed the window for 2.7, so backporting it won't be necessary."
<http://bugs.python.org/issue1578269#msg102631>

[Vincent Ladeuil (vila)]

Since only windows is concerned here (as far as regressions go), I'd prefer that you target 2.2 instead. We won't release 2.0 and 2.1 for windows any more so no need to make our life harder when merging up.

For the news entry, just file it where appropriate in NEWS in the 2.2 branch. When merging up in trunk, don't duplicate the entry for 2.3 but make sure it's added to doc/en/releases-notes/bzr-2.2.txt (the merge should try to add it to bzr-2.3.txt with huge conflicts, just get rid of them).

The fix looks sane enough that I can pre-approve landing it to 2.2 and trunk, but feel free to make two proposals (one for 2.2 and one for trunk) if you prefer a further review.

So my vote is Disapprove for 2.0, but Approve for 2.2 and trunk

review: Disapprove

[Martin Packman (gz)]

I personally use 2.1 which works because I've not yet upgraded to the latest minor version. This isn't an *important* bug, but I don't like regressing behaviour then leaving it in a broken state on older branches.

[Vincent Ladeuil (vila)]

And would you upgrade your 2.1 version if this fix was included ? If you want to make one proposal by targeted version, it would be fine by me, but keep in mind that merging up is pretty awkward (we stopped duplicating NEWS entries in 2.3 but you still have to mention the bug fix in 2.0/2.1/2.2).

[Martin Packman (gz)]

That's a valid question. Maybe, though I do tend only to upgrade if something breaks or I see a change I want.

The approach in this branch of partially backing out the effect of the regressing change is pretty pointless to retarget on trunk, may as well just go straight to fixing the various path functions instead.

[Martin Pool (mbp)]

Let's unblock this.

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

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?

The test is nice.

NEWS may require manual merges, but it shouldn't be inherently very difficult to merge it up.

We might not release from 2.0 again, but I don't particularly see a reason not to land it there.

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

review: Approve

[Martin Packman (gz)]

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.

[Vincent Ladeuil (vila)]

Good to land !

review: Approve

[Vincent Ladeuil (vila)]

sent to pqm by email

=== modified file 'NEWS'
--- NEWS	2010-12-01 10:44:44 +0000
+++ NEWS	2010-12-16 09:04:11 +0000
@@ -25,6 +25,9 @@
 Bug Fixes
 *********
 
+* Avoid UnicodeDecodeError in ``bzr add`` with multiple files under a non-ascii
+  path on windows from symlink support addition. (Martin [gz], #686611)
+
 Improvements
 ************
 

=== modified file 'bzrlib/mutabletree.py'
--- bzrlib/mutabletree.py	2010-07-17 17:16:19 +0000
+++ bzrlib/mutabletree.py	2010-12-16 09:04:11 +0000
@@ -391,7 +391,9 @@
 
         # expand any symlinks in the directory part, while leaving the
         # filename alone
-        file_list = map(osutils.normalizepath, file_list)
+        # only expanding if symlinks are supported avoids windows path bugs
+        if osutils.has_symlinks():
+            file_list = map(osutils.normalizepath, file_list)
 
         # validate user file paths and convert all paths to tree
         # relative : it's cheaper to make a tree relative path an abspath

=== modified file 'bzrlib/tests/blackbox/test_add.py'
--- bzrlib/tests/blackbox/test_add.py	2009-08-10 08:25:05 +0000
+++ bzrlib/tests/blackbox/test_add.py	2010-12-16 09:04:11 +0000
@@ -24,7 +24,8 @@
     condition_isinstance,
     split_suite_by_condition,
     multiply_tests,
-    SymlinkFeature
+    SymlinkFeature,
+    UnicodeFilename,
     )
 from bzrlib.tests.blackbox import ExternalBase
 from bzrlib.tests.test_win32utils import NeedsGlobExpansionFeature
@@ -239,3 +240,12 @@
         os.symlink(osutils.abspath('target'), 'tree/link')
         out = self.run_bzr(['add', 'tree/link'])[0]
         self.assertEquals(out, 'adding link\n')
+
+    def test_add_multiple_files_in_unicode_cwd(self):
+        """Adding multiple files in a non-ascii cwd, see lp:686611"""
+        self.requireFeature(UnicodeFilename)
+        self.make_branch_and_tree(u"\xA7")
+        self.build_tree([u"\xA7/a", u"\xA7/b"])
+        out, err = self.run_bzr(["add", "a", "b"], working_dir=u"\xA7")
+        self.assertEquals(out, "adding a\n" "adding b\n")
+        self.assertEquals(err, "")