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

[Martin Packman]

Add path_from_environ and clean ups to osutils and win32utils

The basic idea behind this branch is to add an interface for retrieving a path from the environment, it's just a little bogged down in osutils/win32utils tech debt.

The main drawback apart from that is that paths really are bytestrings on nix, and in a couple of places can get away with that (at least until they wind up in an exception object or similar). Without a bigger fix for lots of apis, this is pretty intractable unfortunately.

Because a few things need working out still, I've not pushed the bzrlib.config changes, this probably wants coordinating with Jelmer's changes:

<https://code.launchpad.net/~jelmer/bzr/commit-uses-config-stacks/+merge/85134>

The background is this merge proposal:

<https://code.launchpad.net/~vila/bzr/822571-bzr-home-unicode/+merge/70870>

Also want to unblock this one by fixing the various underlying api issues:

<https://code.launchpad.net/~jelmer/bzr/cachedir/+merge/71486>

Proposing despite my confusion for some feedback.

[Vincent Ladeuil (vila)]

20 + # GZ 2011-12-12:Ideally want to include `key` in the exception message
21 + raise errors.BadFilenameEncoding(val, _fs_enc)

Any reason for not defining an exception derived from BadFilenameEncoding or
just make it a bit more customizable ?

24 +def _posix_getuser_unicode():
25 + """Get username from environment or password database as unicode"""
26 + name = getpass.getuser()
27 + user_encoding = get_user_encoding()
28 + try:
29 + return name.decode(user_encoding)
30 + except UnicodeDecodeError:
31 + raise errors.BzrError("Encoding of username %r is unsupported by %s "
32 + "application locale." % (name, user_encoding))

Beware here that we had at least one case (savannah ?) where the /etc/passwd
contained utf-8 names while running under C locale.

The weird thing is that I kind of remember specific code for that... but
can't find it with a quick search. Hints includes: poolie, locks (?) and
pwd...

95 +# GZ 2011-12-12: These tests escape isolation and look at the GlobalConfig
96 +# of the user through lockdir.get_username_for_lock_info
97 class TestLockHeldInfo(TestCase):

Right, use TestCaseInTempDir then ;)

On the other hand, if you have a trick to better detect this kind of issue
(how did you find it in this case ?), let's generalize the check (if
possible, I know I encountered cases in the past where this wasn't
possible).

108 - def test_no_username_bug_660174(self):

Can you just briefly explain why the bug (hence this test) is not relevant
anymore ?

223 - windir = os.environ.get('windir')
224 + windir = get_environ_unicode('WINDIR')

Env vars are not case sensitive on windows ? Worth a reminding comment ;)
Say, in the docstring of the win32utils module ?

Your proposal deletes a lot of 'Returned value can be unicode or plain
string.' which is a huge win ! Thanks.

What's the status now ? We can safely get unicode strings from the
environment, distinguishing paths (fs_encoding) from the rest
(user_encoding), trapping illegal byte strings (good).

From there we chase all remaining call to os.environ.get ?

About "illegal" byte strings, I see two cases:

- paths inside a bzr controlled tree (I don't know a case where such a path
  is used in an env var, so we don't have to care),

- paths containing a bzr controlled tree: sorry guys, we don't support that
  and we abort as soon as we encounter an env var with such a path.

Is that correct ?

If yes, then pending the minor tweaks mentioned above, that's a significant improvement that I'd like to see landed ASAP !

review: Needs Fixing

[Martin Packman (gz)]

> Any reason for not defining an exception derived from BadFilenameEncoding or
> just make it a bit more customizable ?

Mostly because there are still a few cases to be written that could do with a custom error, and I wanted to see what they all look like before working out how to structure the exception classes. So, the current _posix_getuser_unicode exception could do with being something more specific than BzrError, and we need a few more functions like that as well.

> Beware here that we had at least one case (savannah ?) where the /etc/passwd
> contained utf-8 names while running under C locale.
>
> The weird thing is that I kind of remember specific code for that... but
> can't find it with a quick search. Hints includes: poolie, locks (?) and
> pwd...

Yep, it's just moving that existing code, so there's no behaviour change on that front.

> Right, use TestCaseInTempDir then ;)

That's why I threw in the comment, couldn't remember what change is needed so wanted your advice. :)

> On the other hand, if you have a trick to better detect this kind of issue
> (how did you find it in this case ?), let's generalize the check (if
> possible, I know I encountered cases in the past where this wasn't
> possible).

Manually I'm afraid, was seeing if I could replace the deleted test with one that checked the lock breaking code did the right thing if no username could be determined - and was very surprised to see selftest turn up a value from a config file I'd completely forgotten existed. We should be able to isolate, or at least fail on, access to GlobalConfig with some code in TestCase though.

> Can you just briefly explain why the bug (hence this test) is not relevant
> anymore ?

The username is now obtained via the existing win32utils function, which unlike the python getpass module does the right thing when run as a service. Testing that root bug is unfortunately harder than the superficial test added with the previous workaround, we don't have infrastructure in python to run processes as different users on windows.

> Env vars are not case sensitive on windows ? Worth a reminding comment ;)
> Say, in the docstring of the win32utils module ?

Will do. Would still be nice to have envvar code that was aware of this fact for the override in different case issue.

> What's the status now ? We can safely get unicode strings from the
> environment, distinguishing paths (fs_encoding) from the rest
> (user_encoding), trapping illegal byte strings (good).
>
> From there we chase all remaining call to os.environ.get ?

Right, a bunch are just getting a bool or int or something which works fine as is (bytes on nix, lossy downcoding on windows that still preserves the basic ascii characters). The remaining issues are things like BZR_EMAIL which contain text unrelated to filesystem paths but likely in the locale of the user, and os.path.expanduser("~"), which currently need remembering to decode afterwards.

> About "illegal" byte strings, I see two cases:
>
> - paths inside a bzr controlled tree (I don't know a case where such a path
> is used in an env var, so we don't have to care),
>
> - paths containing a bzr controlled tree: sorry guys, w...

[Vincent Ladeuil (vila)]

> Mostly because there are still a few cases to be written that could do
> with a custom error, and I wanted to see what they all look like before
> working out how to structure the exception classes. So, the current
> _posix_getuser_unicode exception could do with being something more
> specific than BzrError, and we need a few more functions like that as
> well.

Right, nothing forbidding creating a specific exception for your use case
here, make it private if you think you'll need to fix it soon.

>
> > Beware here that we had at least one case (savannah ?) where the /etc/passwd
> > contained utf-8 names while running under C locale.
> >
> > The weird thing is that I kind of remember specific code for that... but
> > can't find it with a quick search. Hints includes: poolie, locks (?) and
> > pwd...
>
> Yep, it's just moving that existing code, so there's no behaviour change on
> that front.
>

Ghaa, if I read the code correctly then the bug is still there for
savannah... I cannot find a bug for it :-/

> > Right, use TestCaseInTempDir then ;)
>
> That's why I threw in the comment, couldn't remember what change is needed so
> wanted your advice. :)

Here you are then :)

The rule is: TestCase can only be used for tests that requires no disk
resources. As soon as you start testing stuff sufficiently high in the
stack, you're bound to encounter some config or log access.

>
> > On the other hand, if you have a trick to better detect this kind of issue
> > (how did you find it in this case ?), let's generalize the check (if
> > possible, I know I encountered cases in the past where this wasn't
> > possible).
>

> Manually I'm afraid, was seeing if I could replace the deleted test with one
> that checked the lock breaking code did the right thing if no username could
> be determined - and was very surprised to see selftest turn up a value from a
> config file I'd completely forgotten existed. We should be able to isolate, or
> at least fail on, access to GlobalConfig with some code in TestCase though.

Ha right, the infamous 'HOME is set to None but python is helpful by still
expanding ~'.

Adding configs to TestCase is a double-edge sword, this wil mask some
failures (this one included). Isolation is better achieved by using
TestCaseInTempDir or even TestCaseWithTransport when you should.

[Martin Packman (gz)]

sent to pqm by email

=== modified file 'bzrlib/osutils.py'
--- bzrlib/osutils.py	2011-12-13 17:10:47 +0000
+++ bzrlib/osutils.py	2011-12-14 18:30:32 +0000
@@ -320,6 +320,33 @@
     return path
 
 
+def _posix_path_from_environ(key):
+    """Get unicode path from `key` in environment or None if not present
+
+    Note that posix systems use arbitrary byte strings for filesystem objects,
+    so a path that raises BadFilenameEncoding here may still be accessible.
+    """
+    val = os.environ.get(key, None)
+    if val is None:
+        return val
+    try:
+        return val.decode(_fs_enc)
+    except UnicodeDecodeError:
+        # GZ 2011-12-12:Ideally want to include `key` in the exception message
+        raise errors.BadFilenameEncoding(val, _fs_enc)
+
+
+def _posix_getuser_unicode():
+    """Get username from environment or password database as unicode"""
+    name = getpass.getuser()
+    user_encoding = get_user_encoding()
+    try:
+        return name.decode(user_encoding)
+    except UnicodeDecodeError:
+        raise errors.BzrError("Encoding of username %r is unsupported by %s "
+            "application locale." % (name, user_encoding))
+
+
 def _win32_fixdrive(path):
     """Force drive letters to be consistent.
 
@@ -414,6 +441,8 @@
 realpath = _posix_realpath
 pathjoin = os.path.join
 normpath = _posix_normpath
+path_from_environ = _posix_path_from_environ
+getuser_unicode = _posix_getuser_unicode
 getcwd = os.getcwdu
 rename = os.rename
 dirname = os.path.dirname
@@ -475,6 +504,8 @@
     f = win32utils.get_unicode_argv     # special function or None
     if f is not None:
         get_unicode_argv = f
+    path_from_environ = win32utils.get_environ_unicode
+    getuser_unicode = win32utils.get_user_name
 
 elif sys.platform == 'darwin':
     getcwd = _mac_getcwd
@@ -2431,30 +2462,6 @@
     open_file = open
 
 
-def getuser_unicode():
-    """Return the username as unicode.
-    """
-    try:
-        user_encoding = get_user_encoding()
-        username = getpass.getuser().decode(user_encoding)
-    except UnicodeDecodeError:
-        raise errors.BzrError("Can't decode username as %s." % \
-                user_encoding)
-    except ImportError, e:
-        if sys.platform != 'win32':
-            raise
-        if str(e) != 'No module named pwd':
-            raise
-        # https://bugs.launchpad.net/bzr/+bug/660174
-        # getpass.getuser() is unable to return username on Windows
-        # if there is no USERNAME environment variable set.
-        # That could be true if bzr is running as a service,
-        # e.g. running `bzr serve` as a service on Windows.
-        # We should not fail with traceback in this case.
-        username = u'UNKNOWN'
-    return username
-
-
 def available_backup_name(base, exists):
     """Find a non-existing backup file name.
 

=== modified file 'bzrlib/tests/test_lockdir.py'
--- bzrlib/tests/test_lockdir.py	2011-08-19 22:34:02 +0000
+++ bzrlib/tests/test_lockdir.py	2011-12-14 18:30:32 +0000
@@ -43,6 +43,7 @@
 from bzrlib.tests import (
     features,
     TestCase,
+    TestCaseInTempDir,
     TestCaseWithTransport,
     )
 
@@ -654,7 +655,7 @@
         self.assertEqual([], self._calls)
 
 
-class TestLockHeldInfo(TestCase):
+class TestLockHeldInfo(TestCaseInTempDir):
     """Can get information about the lock holder, and detect whether they're
     still alive."""
 

=== modified file 'bzrlib/tests/test_osutils.py'
--- bzrlib/tests/test_osutils.py	2011-12-14 17:07:11 +0000
+++ bzrlib/tests/test_osutils.py	2011-12-14 18:30:32 +0000
@@ -2115,12 +2115,6 @@
         self.overrideEnv('LOGNAME', u'jrandom\xb6'.encode(ue))
         self.assertEqual(u'jrandom\xb6', osutils.getuser_unicode())
 
-    def test_no_username_bug_660174(self):
-        self.requireFeature(features.win32_feature)
-        for name in ('LOGNAME', 'USER', 'LNAME', 'USERNAME'):
-            self.overrideEnv(name, None)
-        self.assertEqual(u'UNKNOWN', osutils.getuser_unicode())
-
 
 class TestBackupNames(tests.TestCase):
 

=== modified file 'bzrlib/win32utils.py'
--- bzrlib/win32utils.py	2011-12-12 13:31:08 +0000
+++ bzrlib/win32utils.py	2011-12-14 18:30:32 +0000
@@ -20,11 +20,15 @@
 """
 
 import glob
+import operator
 import os
 import struct
 import sys
 
-from bzrlib import cmdline
+from bzrlib import (
+    cmdline,
+    symbol_versioning,
+    )
 from bzrlib.i18n import gettext
 
 # Windows version
@@ -63,9 +67,12 @@
 else:
     if winver == 'Windows 98':
         create_buffer = ctypes.create_string_buffer
+        def extract_buffer(buf):
+            return buf.value.decode("mbcs")
         suffix = 'A'
     else:
         create_buffer = ctypes.create_unicode_buffer
+        extract_buffer = operator.attrgetter("value")
         suffix = 'W'
 try:
     import pywintypes
@@ -248,28 +255,12 @@
     one that moves with the user as they logon to different machines, and
     a 'local' one that stays local to the machine.  This returns the 'roaming'
     directory, and thus is suitable for storing user-preferences, etc.
-
-    Returned value can be unicode or plain string.
-    To convert plain string to unicode use
-    s.decode(osutils.get_user_encoding())
-    (XXX - but see bug 262874, which asserts the correct encoding is 'mbcs')
     """
     appdata = _get_sh_special_folder_path(CSIDL_APPDATA)
     if appdata:
         return appdata
-    # from env variable
-    appdata = os.environ.get('APPDATA')
-    if appdata:
-        return appdata
-    # if we fall to this point we on win98
-    # at least try C:/WINDOWS/Application Data
-    windir = os.environ.get('windir')
-    if windir:
-        appdata = os.path.join(windir, 'Application Data')
-        if os.path.isdir(appdata):
-            return appdata
-    # did not find anything
-    return None
+    # Use APPDATA if defined, will return None if not
+    return get_environ_unicode('APPDATA')
 
 
 def get_local_appdata_location():
@@ -281,17 +272,12 @@
     a 'local' one that stays local to the machine.  This returns the 'local'
     directory, and thus is suitable for caches, temp files and other things
     which don't need to move with the user.
-
-    Returned value can be unicode or plain string.
-    To convert plain string to unicode use
-    s.decode(osutils.get_user_encoding())
-    (XXX - but see bug 262874, which asserts the correct encoding is 'mbcs')
     """
     local = _get_sh_special_folder_path(CSIDL_LOCAL_APPDATA)
     if local:
         return local
     # Vista supplies LOCALAPPDATA, but XP and earlier do not.
-    local = os.environ.get('LOCALAPPDATA')
+    local = get_environ_unicode('LOCALAPPDATA')
     if local:
         return local
     return get_appdata_location()
@@ -302,33 +288,27 @@
     Assume on win32 it's the <My Documents> folder.
     If location cannot be obtained return system drive root,
     i.e. C:\
-
-    Returned value can be unicode or plain string.
-    To convert plain string to unicode use
-    s.decode(osutils.get_user_encoding())
     """
     home = _get_sh_special_folder_path(CSIDL_PERSONAL)
     if home:
         return home
-    # try for HOME env variable
-    home = os.path.expanduser('~')
-    if home != '~':
+    home = get_environ_unicode('HOME')
+    if home is not None:
         return home
+    homepath = get_environ_unicode('HOMEPATH')
+    if homepath is not None:
+        return os.path.join(get_environ_unicode('HOMEDIR', ''), home)
     # at least return windows root directory
-    windir = os.environ.get('windir')
+    windir = get_environ_unicode('WINDIR')
     if windir:
         return os.path.splitdrive(windir)[0] + '/'
     # otherwise C:\ is good enough for 98% users
-    return 'C:/'
+    return unicode('C:/')
 
 
 def get_user_name():
     """Return user name as login name.
     If name cannot be obtained return None.
-
-    Returned value can be unicode or plain string.
-    To convert plain string to unicode use
-    s.decode(osutils.get_user_encoding())
     """
     if has_ctypes:
         try:
@@ -340,9 +320,9 @@
             buf = create_buffer(UNLEN+1)
             n = ctypes.c_int(UNLEN+1)
             if GetUserName(buf, ctypes.byref(n)):
-                return buf.value
+                return extract_buffer(buf)
     # otherwise try env variables
-    return os.environ.get('USERNAME', None)
+    return get_environ_unicode('USERNAME')
 
 
 # 1 == ComputerNameDnsHostname, which returns "The DNS host name of the local
@@ -353,8 +333,7 @@
     """Return host machine name.
     If name cannot be obtained return None.
 
-    :return: A unicode string representing the host name. On win98, this may be
-        a plain string as win32 api doesn't support unicode.
+    :return: A unicode string representing the host name.
     """
     if has_win32api:
         try:
@@ -377,7 +356,7 @@
             if (GetComputerNameEx is not None
                 and GetComputerNameEx(_WIN32_ComputerNameDnsHostname,
                                       buf, ctypes.byref(n))):
-                return buf.value
+                return extract_buffer(buf)
 
             # Try GetComputerName in case GetComputerNameEx wasn't found
             # It returns the NETBIOS name, which isn't as good, but still ok.
@@ -387,18 +366,12 @@
                                       None)
             if (GetComputerName is not None
                 and GetComputerName(buf, ctypes.byref(n))):
-                return buf.value
-    # otherwise try env variables, which will be 'mbcs' encoded
-    # on Windows (Python doesn't expose the native win32 unicode environment)
-    # According to this:
-    # http://msdn.microsoft.com/en-us/library/aa246807.aspx
-    # environment variables should always be encoded in 'mbcs'.
-    try:
-        return os.environ['COMPUTERNAME'].decode("mbcs")
-    except KeyError:
-        return None
-
-
+                return extract_buffer(buf)
+    return get_environ_unicode('COMPUTERNAME')
+
+
+@symbol_versioning.deprecated_method(
+    symbol_versioning.deprecated_in((2, 5, 0)))
 def _ensure_unicode(s):
     if s and type(s) != unicode:
         from bzrlib import osutils
@@ -406,17 +379,17 @@
     return s
 
 
-def get_appdata_location_unicode():
-    return _ensure_unicode(get_appdata_location())
-
-def get_home_location_unicode():
-    return _ensure_unicode(get_home_location())
-
-def get_user_name_unicode():
-    return _ensure_unicode(get_user_name())
-
-def get_host_name_unicode():
-    return _ensure_unicode(get_host_name())
+get_appdata_location_unicode = symbol_versioning.deprecated_method(
+    symbol_versioning.deprecated_in((2, 5, 0)))(get_appdata_location)
+
+get_home_location_unicode = symbol_versioning.deprecated_method(
+    symbol_versioning.deprecated_in((2, 5, 0)))(get_home_location)
+
+get_user_name_unicode = symbol_versioning.deprecated_method(
+    symbol_versioning.deprecated_in((2, 5, 0)))(get_user_name)
+
+get_host_name_unicode = symbol_versioning.deprecated_method(
+    symbol_versioning.deprecated_in((2, 5, 0)))(get_host_name)
 
 
 def _ensure_with_dir(path):
@@ -433,7 +406,6 @@
         return path
 
 
-
 def glob_one(possible_glob):
     """Same as glob.glob().
 
@@ -585,6 +557,11 @@
     def get_environ_unicode(key, default=None):
         """Get `key` from environment as unicode or `default` if unset
 
+        The environment is natively unicode on modern windows versions but
+        Python 2 only accesses it through the legacy bytestring api.
+
+        Environmental variable names are case insenstive on Windows.
+
         A large enough buffer will be allocated to retrieve the value, though
         it may take two calls to the underlying library function.
 
@@ -609,7 +586,16 @@
                 return buffer[:length]
             buffer_size = length
 else:
-    get_unicode_argv = get_environ_unicode = None
+    get_unicode_argv = None
+    def get_environ_unicode(key, default=None):
+        """Get `key` from environment as unicode or `default` if unset
+
+        Fallback version that should basically never be needed.
+        """
+        try:
+            return os.environ[key].decode("mbcs")
+        except KeyError:
+            return default
 
 
 if has_win32api: