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

[Martin Packman]

Add get_environ_unicode for accessing variables without mbcs mangling on windows

Adds a mechanism for getting unicode values from the environment directly on windows without the lossy encode/decode used by Python 2 environ access. For most uses, just doing correct encoding and decoding is enough, but this should be an easier interface to get right with less risk of corrupting the environment block by mistake.

An equivalent function for putting unicode values has not been added. While it's nice to cope with all inputs, it's also best not to export values we know may cause problems for other applications. Also, bypassing the os.environ dict cache is fine in the getenv direction, but in the putenv direction leads means future access will get old values.

[Vincent Ladeuil (vila)]

Hmm, warming up for the fix-the-oldest-bug challenge are you ?

79 + A large enough buffer will be allocated to retrieve the value, unless
80 + unless `_size` is set which will act as a hard limit.

s/unless unless/unless/

84 + f = getattr(get_environ_unicode, "_func", None)

I have a few concerns here:
- 'f' is too short :)
- using function attributes is unusal and could be explained,
- this is addressing a lot of issues that aren't documented ;)

Fix as best as you can so some unaware hacker won't go crazy searching for a mythical '_func' attribute on docs.python.org tricked by the resemblance with say, 'im_func' ;)

Also, teach your win editor about not mixing line endings in test_win32utils ;-p

review: Needs Fixing

[Martin Packman (gz)]

> Hmm, warming up for the fix-the-oldest-bug challenge are you ?

The next one is actually nearly a month newer than this bug. :)

> s/unless unless/unless/

Done... and then pulled anyway, looking at it _size isn't really helpful now I know the buffer logic is correct.

> 84 + f = getattr(get_environ_unicode, "_func", None)
>
> I have a few concerns here:
> - 'f' is too short :)

Changed to `cfunc`.

> - using function attributes is unusal and could be explained,
> - this is addressing a lot of issues that aren't documented ;)
>
> Fix as best as you can so some unaware hacker won't go crazy searching for a
> mythical '_func' attribute on docs.python.org tricked by the resemblance with
> say, 'im_func' ;)

And changed to `._c_function`. I agree some of these issues over how to write ctypes code need documenting properly. I think HACKING may be the best place, but hopefully a general cleanup can happen and make the current code somewhat consistent. I'll leave that to a later branch for now.

> Also, teach your win editor about not mixing line endings in test_win32utils
> ;-p

I have no idea what you're talking about! ...ehem...

[Vincent Ladeuil (vila)]

nudge, nudge, land, land

review: Approve

[Martin Packman (gz)]

sent to pqm by email

=== modified file 'bzrlib/tests/test_win32utils.py'
--- bzrlib/tests/test_win32utils.py	2011-06-14 01:26:41 +0000
+++ bzrlib/tests/test_win32utils.py	2011-12-12 12:26:25 +0000
@@ -328,3 +328,56 @@
         self.assertCommandLine([u"rm", u"x*"], "-m pdb rm x*", ["rm", u"x*"])
         self.assertCommandLine([u"add", u"d/f1", u"d/f2"], "-m pdb add d/*",
             ["add", u"d/*"])
+
+
+class TestGetEnvironUnicode(tests.TestCase):
+    """Tests for accessing the environment via the windows wide api"""
+
+    _test_needs_features = [CtypesFeature, features.win32_feature]
+
+    def setUp(self):
+        super(TestGetEnvironUnicode, self).setUp()
+        self.overrideEnv("TEST", "1")
+
+    def test_get(self):
+        """In the normal case behaves the same as os.environ access"""
+        self.assertEqual("1", win32utils.get_environ_unicode("TEST"))
+
+    def test_unset(self):
+        """A variable not present in the environment gives None by default"""
+        del os.environ["TEST"]
+        self.assertIs(None, win32utils.get_environ_unicode("TEST"))
+
+    def test_unset_default(self):
+        """A variable not present in the environment gives passed default"""
+        del os.environ["TEST"]
+        self.assertIs("a", win32utils.get_environ_unicode("TEST", "a"))
+
+    def test_unicode(self):
+        """A non-ascii variable is returned as unicode"""
+        unicode_val = u"\xa7" # non-ascii character present in many encodings
+        try:
+            bytes_val = unicode_val.encode(osutils.get_user_encoding())
+        except UnicodeEncodeError:
+            self.skip("Couldn't encode non-ascii string to place in environ")
+        os.environ["TEST"] = bytes_val
+        self.assertEqual(unicode_val, win32utils.get_environ_unicode("TEST"))
+
+    def test_long(self):
+        """A variable bigger than heuristic buffer size is still accessible"""
+        big_val = "x" * (2<<10)
+        os.environ["TEST"] = big_val
+        self.assertEqual(big_val, win32utils.get_environ_unicode("TEST"))
+
+    def test_unexpected_error(self):
+        """An error from the underlying platform function is propogated"""
+        ERROR_INVALID_PARAMETER = 87
+        SetLastError = win32utils.ctypes.windll.kernel32.SetLastError
+        def failer(*args, **kwargs):
+            SetLastError(ERROR_INVALID_PARAMETER)
+            return 0
+        self.overrideAttr(win32utils.get_environ_unicode, "_c_function",
+            failer)
+        e = self.assertRaises(WindowsError,
+            win32utils.get_environ_unicode, "TEST")
+        self.assertEqual(e.winerror, ERROR_INVALID_PARAMETER)

=== modified file 'bzrlib/win32utils.py'
--- bzrlib/win32utils.py	2011-12-05 11:06:16 +0000
+++ bzrlib/win32utils.py	2011-12-12 12:26:25 +0000
@@ -569,7 +569,7 @@
     return args
 
 
-if has_ctypes and winver != 'Windows 98':
+if has_ctypes and winver == 'Windows NT':
     def get_unicode_argv():
         prototype = ctypes.WINFUNCTYPE(ctypes.c_wchar_p)
         GetCommandLineW = prototype(("GetCommandLineW",
@@ -580,8 +580,36 @@
         # Skip the first argument, since we only care about parameters
         argv = _command_line_to_argv(command_line, sys.argv)[1:]
         return argv
+    
+
+    def get_environ_unicode(key, default=None):
+        """Get `key` from environment as unicode or `default` if unset
+
+        A large enough buffer will be allocated to retrieve the value, though
+        it may take two calls to the underlying library function.
+
+        This needs ctypes because pywin32 does not expose the wide version.
+        """
+        cfunc = getattr(get_environ_unicode, "_c_function", None)
+        if cfunc is None:
+            from ctypes.wintypes import DWORD, LPCWSTR, LPWSTR
+            cfunc = ctypes.WINFUNCTYPE(DWORD, LPCWSTR, LPWSTR, DWORD)(
+                ("GetEnvironmentVariableW", ctypes.windll.kernel32))
+            get_environ_unicode._c_function = cfunc
+        buffer_size = 256 # heuristic, 256 characters often enough
+        while True:
+            buffer = ctypes.create_unicode_buffer(buffer_size)
+            length = cfunc(key, buffer, buffer_size)
+            if not length:
+                code = ctypes.GetLastError()
+                if code == 203: # ERROR_ENVVAR_NOT_FOUND
+                    return default
+                raise ctypes.WinError(code)
+            if buffer_size > length:
+                return buffer[:length]
+            buffer_size = length
 else:
-    get_unicode_argv = None
+    get_unicode_argv = get_environ_unicode = None
 
 
 if has_win32api: