Merge lp:~gz/bzr/filesystem_default_encoding_794353 into lp:bzr

[Martin Packman]

Override Py_FileSystemDefaultEncoding to utf-8 from ascii for the bzr script

Use dark ctypes hackery to change the encoding Python uses when interacting with the filesystem on non-OSX posix machines. This affects open() and most os module functions when passed a unicode filename. Existing locale settings are obeyed, but if an invalid locale or the C locale is given, UTF-8 will be used instead of ascii for encoding and decoding filenames and such like.

Looking through the Python 2.7 source, I believe this is a safe operation. The Py_FileSystemDefaultEncoding global is declared in Python/bltinmodule.c and initialised on posix systems in Python/pythonrun.c if a valid locale is given. It is accessed in Objects/fileobject.c for open(), in Python/import.c for imp.find_module(), in Python/sysmodule.c to return the value with sys.getfilesystemencoding(), in Modules/_io/fileio.c for _io.FileIO() which is the Python 3 io backport, and throughout Modules/posixmodule.c for all the os functions. Nothing looks like it will cause issues if the value is changed underneath. On the bzrlib side, doing this early in startup should avoid any potential problems.

Initially this is likely to just expose a lot of areas where the ui does not handle encoding fallbacks correctly, and the user encoding will still be ascii. For instance, in the test, replacing "init" with "init-repo" results in a UnicodeEncodeError. I don't think changing the user encoding default as well is necessarily the correct thing, and most issues like this are existing usability problems when running on a non-utf-8 terminal.

[Jelmer Vernooij (jelmer)]

Do we really want to do this in bzrlib.__init__ ? This will also influence other applications that happen to use bzrlib. Changing the filesystem encoding behind their back seems wrong.

Perhaps it would make more sense to do this in the 'bzr' script?

review: Needs Information

[Jelmer Vernooij (jelmer)]

Garr, nevermind. Jelmer skimming fail. I will lay off the coffee and try to read slower. Sorry.

[Jelmer Vernooij (jelmer)]

review: Approve (code)

[Vincent Ladeuil (vila)]

Based on the tests and the decrease of roundtrips: approved !

16 + def _vfs_checkout_metadir(self):
17 + self._ensure_real()
18 + return self._real_bzrdir.checkout_metadir()
19 +
20 + def checkout_metadir(self):
21 + medium = self._client._medium
22 + if medium._is_remote_before((2, 5)):
23 + return self._vfs_checkout_metadir()

_vfs_checkout_metadir -> checkout_metadir -> _vfs_checkout_metadir ?

Is that a loop or do I miss something obvious ?

Up to you to wait for spiv's review.

review: Approve

[Vincent Ladeuil (vila)]

Meh, wrong proposal.

[Martin Packman (gz)]

sent to pqm by email

[Martin Packman (gz)]

sent to pqm by email

[Martin Packman (gz)]

sent to pqm by email

[Martin Packman (gz)]

sent to pqm by email

=== modified file 'bzr'
--- bzr	2011-10-27 15:38:14 +0000
+++ bzr	2011-12-14 17:09:40 +0000
@@ -84,6 +84,10 @@
             '  Although this should be no problem for bzr itself, it might\n'
             '  cause problems with some plugins. To investigate the issue,\n'
             '  look at the output of the locale(1p) tool.\n' % e)
+    # Use better default than ascii with posix filesystems that deal in bytes
+    # natively even when the C locale or no locale at all is given. Note that
+    # we need an immortal string for the hack, hence the lack of a hyphen.
+    sys._bzr_default_fs_enc = "utf8"
 
 
 # The python2.6 release includes some libraries that have deprecation warnings

=== modified file 'bzrlib/__init__.py'
--- bzrlib/__init__.py	2011-12-08 09:24:06 +0000
+++ bzrlib/__init__.py	2011-12-14 17:09:40 +0000
@@ -37,6 +37,7 @@
 # timestamps relative to program start in the log file kept by bzrlib.trace.
 _start_time = time.time()
 
+import codecs
 import sys
 
 
@@ -131,6 +132,47 @@
 __version__ = _format_version_tuple(version_info)
 version_string = __version__
 
+
+def _patch_filesystem_default_encoding(new_enc):
+    """Change the Python process global encoding for filesystem names
+    
+    The effect is to change how open() and other builtin functions handle
+    unicode filenames on posix systems. This should only be done near startup.
+
+    The new encoding string passed to this function must survive until process
+    termination, otherwise the interpreter may access uninitialized memory.
+    The use of intern() may defer breakage is but is not enough, the string
+    object should be secure against module reloading and during teardown.
+    """
+    try:
+        import ctypes
+    except ImportError:
+        return
+    pythonapi = getattr(ctypes, "pythonapi", None)
+    if pythonapi is None:
+        # Not CPython ctypes implementation
+        return
+    old_ptr = ctypes.c_void_p.in_dll(pythonapi, "Py_FileSystemDefaultEncoding")
+    new_ptr = ctypes.cast(ctypes.c_char_p(intern(new_enc)), ctypes.c_void_p)
+    old_ptr.value = new_ptr.value
+    if sys.getfilesystemencoding() != new_enc:
+        raise RuntimeError("Failed to change the filesystem default encoding")
+    return new_enc
+
+
+# When running under the bzr script, override bad filesystem default encoding.
+# This is not safe to do for all users of bzrlib, other scripts should instead
+# just ensure a usable locale is set via the $LANG variable on posix systems.
+_fs_enc = sys.getfilesystemencoding()
+if getattr(sys, "_bzr_default_fs_enc", None) is not None:
+    if (_fs_enc is None or codecs.lookup(_fs_enc).name == "ascii"):
+        _fs_enc = _patch_filesystem_default_encoding(sys._bzr_default_fs_enc)
+if _fs_enc is None:
+    _fs_enc = "ascii"
+else:
+    _fs_enc = codecs.lookup(_fs_enc).name
+
+
 # bzr has various bits of global state that are slowly being eliminated.
 # This variable is intended to permit any new state-like things to be attached
 # to a library_state.BzrLibraryState object rather than getting new global

=== modified file 'bzrlib/osutils.py'
--- bzrlib/osutils.py	2011-12-05 14:21:55 +0000
+++ bzrlib/osutils.py	2011-12-14 17:09:40 +0000
@@ -63,7 +63,7 @@
 
 
 import bzrlib
-from bzrlib import symbol_versioning
+from bzrlib import symbol_versioning, _fs_enc
 
 
 # Cross platform wall-clock time functionality with decent resolution.
@@ -293,7 +293,6 @@
 # choke on a Unicode string containing a relative path if
 # os.getcwd() returns a non-sys.getdefaultencoding()-encoded
 # string.
-_fs_enc = sys.getfilesystemencoding() or 'utf-8'
 def _posix_abspath(path):
     # jam 20060426 rather than encoding to fsencoding
     # copy posixpath.abspath, but use os.getcwdu instead
@@ -1771,7 +1770,6 @@
     """
     global _selected_dir_reader
     if _selected_dir_reader is None:
-        fs_encoding = _fs_enc.upper()
         if sys.platform == "win32" and win32utils.winver == 'Windows NT':
             # Win98 doesn't have unicode apis like FindFirstFileW
             # TODO: We possibly could support Win98 by falling back to the
@@ -1783,8 +1781,7 @@
                 _selected_dir_reader = Win32ReadDir()
             except ImportError:
                 pass
-        elif fs_encoding in ('UTF-8', 'US-ASCII', 'ANSI_X3.4-1968'):
-            # ANSI_X3.4-1968 is a form of ASCII
+        elif _fs_enc in ('utf-8', 'ascii'):
             try:
                 from bzrlib._readdir_pyx import UTF8DirReader
                 _selected_dir_reader = UTF8DirReader()

=== modified file 'bzrlib/tests/blackbox/test_exceptions.py'
--- bzrlib/tests/blackbox/test_exceptions.py	2011-09-16 20:42:31 +0000
+++ bzrlib/tests/blackbox/test_exceptions.py	2011-12-14 17:09:40 +0000
@@ -60,6 +60,14 @@
             flags=re.MULTILINE)
         self.assertEquals(out, "")
 
+    def test_utf8_default_fs_enc(self):
+        """In the C locale bzr treats a posix filesystem as UTF-8 encoded"""
+        if os.name != "posix":
+            raise tests.TestNotApplicable("Needs system beholden to C locales")
+        out, err = self.run_bzr_subprocess(["init", "file:%C2%A7"],
+            env_changes={"LANG": "C", "LC_ALL": "C"})
+        self.assertContainsRe(out, "^Created a standalone tree .*$")
+
 
 class TestOptParseBugHandling(tests.TestCase):
     "Test that we handle http://bugs.python.org/issue2931"

=== modified file 'bzrlib/tests/test_osutils.py'
--- bzrlib/tests/test_osutils.py	2011-12-02 12:49:48 +0000
+++ bzrlib/tests/test_osutils.py	2011-12-14 17:09:40 +0000
@@ -1217,7 +1217,7 @@
         self.requireFeature(UTF8DirReaderFeature)
         self._save_platform_info()
         win32utils.winver = None # Avoid the win32 detection code
-        osutils._fs_enc = 'UTF-8'
+        osutils._fs_enc = 'utf-8'
         self.assertDirReaderIs(
             UTF8DirReaderFeature.module.UTF8DirReader)
 
@@ -1225,22 +1225,14 @@
         self.requireFeature(UTF8DirReaderFeature)
         self._save_platform_info()
         win32utils.winver = None # Avoid the win32 detection code
-        osutils._fs_enc = 'US-ASCII'
-        self.assertDirReaderIs(
-            UTF8DirReaderFeature.module.UTF8DirReader)
-
-    def test_force_walkdirs_utf8_fs_ANSI(self):
-        self.requireFeature(UTF8DirReaderFeature)
-        self._save_platform_info()
-        win32utils.winver = None # Avoid the win32 detection code
-        osutils._fs_enc = 'ANSI_X3.4-1968'
+        osutils._fs_enc = 'ascii'
         self.assertDirReaderIs(
             UTF8DirReaderFeature.module.UTF8DirReader)
 
     def test_force_walkdirs_utf8_fs_latin1(self):
         self._save_platform_info()
         win32utils.winver = None # Avoid the win32 detection code
-        osutils._fs_enc = 'latin1'
+        osutils._fs_enc = 'iso-8859-1'
         self.assertDirReaderIs(osutils.UnicodeDirReader)
 
     def test_force_walkdirs_utf8_nt(self):

=== modified file 'doc/en/release-notes/bzr-2.5.txt'
--- doc/en/release-notes/bzr-2.5.txt	2011-12-14 12:27:44 +0000
+++ doc/en/release-notes/bzr-2.5.txt	2011-12-14 17:09:40 +0000
@@ -36,6 +36,11 @@
 
 * New HPSS call for ``Repository.reconcile``. (Jelmer Vernooij, #894455)
 
+* Override the value returned by ``sys.getfilesystemencoding()`` for the bzr
+  script to utf-8 when it would otherwise be ascii on a posix system. This
+  will mean bzr works with non-ascii files when no locale or an incorrect
+  locale is set. (Martin Packman, #794353)
+
 Bug Fixes
 *********
 

=== modified file 'doc/en/whats-new/whats-new-in-2.5.txt'
--- doc/en/whats-new/whats-new-in-2.5.txt	2011-10-06 14:39:49 +0000
+++ doc/en/whats-new/whats-new-in-2.5.txt	2011-12-14 17:09:40 +0000
@@ -28,6 +28,13 @@
 format is ``long``). This a work in progress and only some options are
 supported so far.
 
+Working on a posix system without a locale
+******************************************
+
+Previously bzr needed a valid locale set to work with branches containing
+non-ascii filenames. It will now use utf-8 rather than ascii as a fallback
+encoding for interacting with the filesystem. This makes creating a working
+tree and commiting to it possible for such branches in most environments.
 
 Further information
 *******************