Merge lp:~mbp/bzr/659763-non-ascii-gecos into lp:bzr/2.3

[Martin Pool]

Add a test for decoding on non-ascii GECOS entries; Fix mis-imported mutter statements.

vila was right that my fix <https://code.launchpad.net/~mbp/bzr/whoami/+merge/55679> also fixed bug 659763. This adds a test for it, cleans up the code a bit, and fixes some (apparently previously untested) broken mutter statements. Because of the last point I think this should go into 2.3 too.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 4/1/2011 6:04 AM, Martin Pool wrote:
> Martin Pool has proposed merging lp:~mbp/bzr/659763-non-ascii-gecos into lp:bzr/2.3.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> Bug #659763 in Bazaar: "bzr smart server can't handle UTF-8 user names, gives UnknownErrorFromSmartServer"
> https://bugs.launchpad.net/bzr/+bug/659763
>
> For more details, see:
> https://code.launchpad.net/~mbp/bzr/659763-non-ascii-gecos/+merge/55862
>
> vila was right that my fix <https://code.launchpad.net/~mbp/bzr/whoami/+merge/55679> also fixed bug 659763. This adds a test for it, cleans up the code a bit, and fixes some (apparently previously untested) broken mutter statements. Because of the last point I think this should go into 2.3 too.

> + def test_auto_user_id(self):
> + """Automatic inference of user name.
> +
> + This is a bit hard to test in an isolated way, because it depends on
> + system functions that go direct to /etc or perhaps somewhere else.
> + But it's reasonable to say that on Unix, with an /etc/mailname, we ought
> + to be able to choose a user name with no configuration.
> + """
> + if sys.platform == 'win32':
> + raise TestSkipped("User name inference not implemented on win32")

This looks more like a missing feature than just a test skipped. (as in,
we don't yet support the feature on windows, or some such).

Off-hand I'm thinking we could use the MAPI support to get an email, and
the username is usually pretty easy to get. Though I don't know it offhand.

> + def test_get_user_name_from_os_smoke(self):
> + """get_user_name_from_pwd may not work but it must not crash."""
> + realname, local_username = config.get_user_name_from_os()
> + self.assertIsInstance(realname, (basestring, type(None)))
> + self.assertIsInstance(local_username, (basestring, type(None)))
> +

^- shouldn't these always be Unicode or None ?

Otherwise looks good to me.

 merge: approve

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk2ViK8ACgkQJdeBCYSNAAP75gCgm5oAGhBjrnksh78XgJ3l1qna
39sAn37HIwKPvSVgXrfgwtQTYQVr7AjR
=GW8X
-----END PGP SIGNATURE-----

review: Approve

[Martin Pool (mbp)]

You're right: the auto_user_id should be marked as a known failure on
windows; and the other one should insist on unicode (which may need a
code fix.)

Martin

[Andrew Bennetts (spiv)]

sent to pqm by email

[Jelmer Vernooij (jelmer)]

sent to pqm by email

[Andrew Bennetts (spiv)]

PQM replied to me: Conflicts during merge: Text conflict in doc/en/release-notes/bzr-2.3.txt

Unmerged revisions

5125. By Martin Pool on 2011-04-01

Add a test for decoding on non-ascii GECOS entries.

Separate out get_user_name_from_os so that it can be tested in isolation.

Fix mis-imported mutter statements.

Add static sampledata into bzrlib.tests.fixtures.

=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2011-01-12 18:12:58 +0000
+++ bzrlib/config.py	2011-04-01 04:04:23 +0000
@@ -63,6 +63,7 @@
 """
 
 import os
+import string
 import sys
 
 from bzrlib import commands
@@ -271,21 +272,21 @@
         the concrete policy type is checked, and finally
         $EMAIL is examined.
         If no username can be found, errors.NoWhoami exception is raised.
-
-        TODO: Check it's reasonably well-formed.
         """
         v = os.environ.get('BZR_EMAIL')
         if v:
             return v.decode(osutils.get_user_encoding())
-
         v = self._get_user_id()
         if v:
             return v
-
         v = os.environ.get('EMAIL')
         if v:
             return v.decode(osutils.get_user_encoding())
-
+        name, email = _auto_user_id()
+        if name and email:
+            return '%s <%s>' % (name, email)
+        elif email:
+            return email
         raise errors.NoWhoami()
 
     def ensure_username(self):
@@ -1213,6 +1214,92 @@
         return os.path.expanduser('~/.cache')
 
 
+def _get_default_mail_domain():
+    """If possible, return the assumed default email domain.
+
+    :returns: string mail domain, or None.
+    """
+    if sys.platform == 'win32':
+        # No implementation yet; patches welcome
+        return None
+    try:
+        f = open('/etc/mailname')
+    except (IOError, OSError), e:
+        return None
+    try:
+        domain = f.read().strip()
+        return domain
+    finally:
+        f.close()
+
+
+def _auto_user_id():
+    """Calculate automatic user identification.
+
+    :returns: (realname, email), either of which may be None if they can't be
+    determined.
+
+    Only used when none is set in the environment or the id file.
+
+    This only returns an email address if we can be fairly sure the 
+    address is reasonable, ie if /etc/mailname is set on unix.
+
+    This doesn't use the FQDN as the default domain because that may be 
+    slow, and it doesn't use the hostname alone because that's not normally 
+    a reasonable address.
+    """
+    if sys.platform == 'win32':
+        # No implementation to reliably determine Windows default mail
+        # address; please add one.
+        return None, None
+
+    default_mail_domain = _get_default_mail_domain()
+    if not default_mail_domain:
+        return None, None
+    realname, username = get_user_name_from_os()
+    return realname, (username + '@' + default_mail_domain)
+
+
+def get_user_name_from_os():
+    """Get information about the user from /etc/passwd or similar.
+
+    Used if no identity is defined.
+
+    :returns: (realname, local_username) or (None, None) if they can't be
+        determined.
+    """
+    if sys.platform == 'win32':
+        # win32 implementation welcome.
+        return None, None
+
+    import pwd
+    uid = os.getuid()
+    try:
+        w = pwd.getpwuid(uid)
+    except KeyError:
+        trace.mutter('no passwd entry for uid %d?' % uid)
+        return None, None
+
+    # we try utf-8 first, because on many variants (like Linux),
+    # /etc/passwd "should" be in utf-8, and because it's unlikely to give
+    # false positives.  (many users will have their user encoding set to
+    # latin-1, which cannot raise UnicodeError.)
+    for encoding in ['utf-8', osutils.get_user_encoding()]:
+        try:
+            gecos = w.pw_gecos.decode(encoding)
+            username = w.pw_name.decode(encoding)
+        except UnicodeError, e:
+            trace.mutter("cannot decode passwd entry %r as %r" %
+                ((w.pw_name, w.pw_gecos,), encoding))
+            continue
+        else:
+            break
+    else:
+        return None, None
+    realname = gecos.split(',')[0]
+    return realname, username
+
+
 def parse_username(username):
     """Parse e-mail username and return a (name, address) tuple."""
     match = re.match(r'(.*?)\s*<?([\w+.-]+@[\w+.-]+)>?', username)

=== modified file 'bzrlib/tests/features.py'
--- bzrlib/tests/features.py	2011-01-12 01:01:53 +0000
+++ bzrlib/tests/features.py	2011-04-01 04:04:23 +0000
@@ -45,6 +45,7 @@
 pywintypes = tests.ModuleAvailableFeature('pywintypes')
 sphinx = tests.ModuleAvailableFeature('sphinx')
 subunit = tests.ModuleAvailableFeature('subunit')
+pwd_module = tests.ModuleAvailableFeature('pwd')
 
 
 class _BackslashDirSeparatorFeature(tests.Feature):

=== modified file 'bzrlib/tests/fixtures.py'
--- bzrlib/tests/fixtures.py	2010-06-26 01:07:16 +0000
+++ bzrlib/tests/fixtures.py	2011-04-01 04:04:23 +0000
@@ -97,3 +97,15 @@
     def __exit__(self, exc_type, exc_val, exc_tb):
         self._calls.append('__exit__')
         return False # propogate exceptions.
+
+
+unicode_real_names = [
+    u'joe user \u2460',
+    u'joe user \u2461',
+    ]
+
+"""local-part of user email addressess"""
+user_names = [
+    'user1',
+    'user2',
+    ]

=== modified file 'bzrlib/tests/test_config.py'
--- bzrlib/tests/test_config.py	2011-02-22 09:49:45 +0000
+++ bzrlib/tests/test_config.py	2011-04-01 04:04:23 +0000
@@ -41,6 +41,8 @@
     )
 from bzrlib.tests import (
     features,
+    fixtures,
+    TestSkipped,
     scenarios,
     )
 from bzrlib.util.configobj import configobj
@@ -2237,3 +2239,50 @@
 # test_user_prompted ?
 class TestAuthenticationRing(tests.TestCaseWithTransport):
     pass
+
+
+class TestAutoUserId(tests.TestCase):
+    """Test inferring an automatic user name."""
+
+    def test_auto_user_id(self):
+        """Automatic inference of user name.
+        
+        This is a bit hard to test in an isolated way, because it depends on
+        system functions that go direct to /etc or perhaps somewhere else.
+        But it's reasonable to say that on Unix, with an /etc/mailname, we ought
+        to be able to choose a user name with no configuration.
+        """
+        if sys.platform == 'win32':
+            raise TestSkipped("User name inference not implemented on win32")
+        realname, address = config._auto_user_id()
+        if os.path.exists('/etc/mailname'):
+            self.assertTrue(realname)
+            self.assertTrue(address)
+        else:
+            self.assertEquals((None, None), (realname, address))
+
+    def test_get_user_name_from_os_smoke(self):
+        """get_user_name_from_pwd may not work but it must not crash."""
+        realname, local_username = config.get_user_name_from_os()
+        self.assertIsInstance(realname, (basestring, type(None)))
+        self.assertIsInstance(local_username, (basestring, type(None)))
+
+    def test_utf8_gecos(self):
+        """Get user name/address from gecos.
+
+        :see: <https://bugs.launchpad.net/bzr/+bug/659763>.
+        """
+        self.requireFeature(features.pwd_module)
+        import pwd
+        fake_gecos = (
+            '%s,,,,' % (fixtures.unicode_real_names[0])).encode('utf-8')
+        uid = os.getuid()
+        def fake_getpwuid(u):
+            if u == uid:
+                return type('FakeGecos', (),
+                    dict(pw_gecos=fake_gecos,
+                        pw_name=fixtures.user_names[0]))
+        self.overrideAttr(pwd, 'getpwuid', fake_getpwuid)
+        realname, username = config.get_user_name_from_os()
+        self.assertEquals(fixtures.unicode_real_names[0], realname)
+        self.assertEquals(fixtures.user_names[0], username)

=== modified file 'doc/en/release-notes/bzr-2.3.txt'
--- doc/en/release-notes/bzr-2.3.txt	2011-04-01 01:37:43 +0000
+++ doc/en/release-notes/bzr-2.3.txt	2011-04-01 04:04:23 +0000
@@ -39,15 +39,22 @@
 .. Fixes for situations where bzr would previously crash or give incorrect
    or undesirable results.
 
-* When reporting a crash without apport, don't print the full list of
-   plugins because it's often too long.
-   (Martin Pool, #716389)
+* Bazaar now infers the default user email address on Unix from the local
+  account name plus the contents of ``/etc/mailname`` if that file exists.
+  In particular, this means that committing as root through etckeeper will
+  normally not require running ``bzr whoami`` first.
+  (Martin Pool, #616878)
 
 * ``bzr push`` into a repository (that doesn't have a branch), will no
   longer copy all revisions in the repository. Only the ones in the
   ancestry of the source branch, like it does in all other cases.
   (John Arbash Meinel, #465517)
 
+* Correctly handle user real names in ``/etc/passwd`` that contain
+  non-ascii characters; these are attempted to be interpreted as either
+  UTF-8 or the user's own encoding.
+  (Martin Pool, #659763)
+
 * Fix ``UnboundLocalError: local variable 'lock_url' in wait_lock`` error,
   especially while trying to save configuration from QBzr.
   (Martin Pool, #733136)
@@ -56,6 +63,10 @@
   had changed.  Bazaar was attempting to open and lock the master branch
   twice in this case.  (Andrew Bennetts, #733350)
 
+* When reporting a crash without apport, don't print the full list of
+  plugins because it's often too long.
+  (Martin Pool, #716389)
+
 Documentation
 *************