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

Proposed by Martin Pool
Status: Superseded
Proposed branch: lp:~mbp/bzr/659763-non-ascii-gecos
Merge into: lp:bzr/2.3
Diff against target: 267 lines (+168/-8)
5 files modified
bzrlib/config.py (+92/-5)
bzrlib/tests/features.py (+1/-0)
bzrlib/tests/fixtures.py (+12/-0)
bzrlib/tests/test_config.py (+49/-0)
doc/en/release-notes/bzr-2.3.txt (+14/-3)
To merge this branch: bzr merge lp:~mbp/bzr/659763-non-ascii-gecos
Reviewer Review Type Date Requested Status
John A Meinel Approve
Review via email: mp+55862@code.launchpad.net

This proposal has been superseded by a proposal from 2011-04-11.

Commit message

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

Description of the change

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.

To post a comment you must log in.
Revision history for this message
John A Meinel (jameinel) wrote :

-----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
Revision history for this message
Martin Pool (mbp) wrote :

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

Revision history for this message
Andrew Bennetts (spiv) wrote :

sent to pqm by email

Revision history for this message
Jelmer Vernooij (jelmer) wrote :

sent to pqm by email

Revision history for this message
Andrew Bennetts (spiv) wrote :

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

Unmerged revisions

5125. By Martin Pool

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
63"""63"""
6464
65import os65import os
66import string
66import sys67import sys
6768
68from bzrlib import commands69from bzrlib import commands
@@ -271,21 +272,21 @@
271 the concrete policy type is checked, and finally272 the concrete policy type is checked, and finally
272 $EMAIL is examined.273 $EMAIL is examined.
273 If no username can be found, errors.NoWhoami exception is raised.274 If no username can be found, errors.NoWhoami exception is raised.
274
275 TODO: Check it's reasonably well-formed.
276 """275 """
277 v = os.environ.get('BZR_EMAIL')276 v = os.environ.get('BZR_EMAIL')
278 if v:277 if v:
279 return v.decode(osutils.get_user_encoding())278 return v.decode(osutils.get_user_encoding())
280
281 v = self._get_user_id()279 v = self._get_user_id()
282 if v:280 if v:
283 return v281 return v
284
285 v = os.environ.get('EMAIL')282 v = os.environ.get('EMAIL')
286 if v:283 if v:
287 return v.decode(osutils.get_user_encoding())284 return v.decode(osutils.get_user_encoding())
288285 name, email = _auto_user_id()
286 if name and email:
287 return '%s <%s>' % (name, email)
288 elif email:
289 return email
289 raise errors.NoWhoami()290 raise errors.NoWhoami()
290291
291 def ensure_username(self):292 def ensure_username(self):
@@ -1213,6 +1214,92 @@
1213 return os.path.expanduser('~/.cache')1214 return os.path.expanduser('~/.cache')
12141215
12151216
1217def _get_default_mail_domain():
1218 """If possible, return the assumed default email domain.
1219
1220 :returns: string mail domain, or None.
1221 """
1222 if sys.platform == 'win32':
1223 # No implementation yet; patches welcome
1224 return None
1225 try:
1226 f = open('/etc/mailname')
1227 except (IOError, OSError), e:
1228 return None
1229 try:
1230 domain = f.read().strip()
1231 return domain
1232 finally:
1233 f.close()
1234
1235
1236def _auto_user_id():
1237 """Calculate automatic user identification.
1238
1239 :returns: (realname, email), either of which may be None if they can't be
1240 determined.
1241
1242 Only used when none is set in the environment or the id file.
1243
1244 This only returns an email address if we can be fairly sure the
1245 address is reasonable, ie if /etc/mailname is set on unix.
1246
1247 This doesn't use the FQDN as the default domain because that may be
1248 slow, and it doesn't use the hostname alone because that's not normally
1249 a reasonable address.
1250 """
1251 if sys.platform == 'win32':
1252 # No implementation to reliably determine Windows default mail
1253 # address; please add one.
1254 return None, None
1255
1256 default_mail_domain = _get_default_mail_domain()
1257 if not default_mail_domain:
1258 return None, None
1259 realname, username = get_user_name_from_os()
1260 return realname, (username + '@' + default_mail_domain)
1261
1262
1263def get_user_name_from_os():
1264 """Get information about the user from /etc/passwd or similar.
1265
1266 Used if no identity is defined.
1267
1268 :returns: (realname, local_username) or (None, None) if they can't be
1269 determined.
1270 """
1271 if sys.platform == 'win32':
1272 # win32 implementation welcome.
1273 return None, None
1274
1275 import pwd
1276 uid = os.getuid()
1277 try:
1278 w = pwd.getpwuid(uid)
1279 except KeyError:
1280 trace.mutter('no passwd entry for uid %d?' % uid)
1281 return None, None
1282
1283 # we try utf-8 first, because on many variants (like Linux),
1284 # /etc/passwd "should" be in utf-8, and because it's unlikely to give
1285 # false positives. (many users will have their user encoding set to
1286 # latin-1, which cannot raise UnicodeError.)
1287 for encoding in ['utf-8', osutils.get_user_encoding()]:
1288 try:
1289 gecos = w.pw_gecos.decode(encoding)
1290 username = w.pw_name.decode(encoding)
1291 except UnicodeError, e:
1292 trace.mutter("cannot decode passwd entry %r as %r" %
1293 ((w.pw_name, w.pw_gecos,), encoding))
1294 continue
1295 else:
1296 break
1297 else:
1298 return None, None
1299 realname = gecos.split(',')[0]
1300 return realname, username
1301
1302
1216def parse_username(username):1303def parse_username(username):
1217 """Parse e-mail username and return a (name, address) tuple."""1304 """Parse e-mail username and return a (name, address) tuple."""
1218 match = re.match(r'(.*?)\s*<?([\w+.-]+@[\w+.-]+)>?', username)1305 match = re.match(r'(.*?)\s*<?([\w+.-]+@[\w+.-]+)>?', username)
12191306
=== 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 @@
45pywintypes = tests.ModuleAvailableFeature('pywintypes')45pywintypes = tests.ModuleAvailableFeature('pywintypes')
46sphinx = tests.ModuleAvailableFeature('sphinx')46sphinx = tests.ModuleAvailableFeature('sphinx')
47subunit = tests.ModuleAvailableFeature('subunit')47subunit = tests.ModuleAvailableFeature('subunit')
48pwd_module = tests.ModuleAvailableFeature('pwd')
4849
4950
50class _BackslashDirSeparatorFeature(tests.Feature):51class _BackslashDirSeparatorFeature(tests.Feature):
5152
=== 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 @@
97 def __exit__(self, exc_type, exc_val, exc_tb):97 def __exit__(self, exc_type, exc_val, exc_tb):
98 self._calls.append('__exit__')98 self._calls.append('__exit__')
99 return False # propogate exceptions.99 return False # propogate exceptions.
100
101
102unicode_real_names = [
103 u'joe user \u2460',
104 u'joe user \u2461',
105 ]
106
107"""local-part of user email addressess"""
108user_names = [
109 'user1',
110 'user2',
111 ]
100112
=== 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 @@
41 )41 )
42from bzrlib.tests import (42from bzrlib.tests import (
43 features,43 features,
44 fixtures,
45 TestSkipped,
44 scenarios,46 scenarios,
45 )47 )
46from bzrlib.util.configobj import configobj48from bzrlib.util.configobj import configobj
@@ -2237,3 +2239,50 @@
2237# test_user_prompted ?2239# test_user_prompted ?
2238class TestAuthenticationRing(tests.TestCaseWithTransport):2240class TestAuthenticationRing(tests.TestCaseWithTransport):
2239 pass2241 pass
2242
2243
2244class TestAutoUserId(tests.TestCase):
2245 """Test inferring an automatic user name."""
2246
2247 def test_auto_user_id(self):
2248 """Automatic inference of user name.
2249
2250 This is a bit hard to test in an isolated way, because it depends on
2251 system functions that go direct to /etc or perhaps somewhere else.
2252 But it's reasonable to say that on Unix, with an /etc/mailname, we ought
2253 to be able to choose a user name with no configuration.
2254 """
2255 if sys.platform == 'win32':
2256 raise TestSkipped("User name inference not implemented on win32")
2257 realname, address = config._auto_user_id()
2258 if os.path.exists('/etc/mailname'):
2259 self.assertTrue(realname)
2260 self.assertTrue(address)
2261 else:
2262 self.assertEquals((None, None), (realname, address))
2263
2264 def test_get_user_name_from_os_smoke(self):
2265 """get_user_name_from_pwd may not work but it must not crash."""
2266 realname, local_username = config.get_user_name_from_os()
2267 self.assertIsInstance(realname, (basestring, type(None)))
2268 self.assertIsInstance(local_username, (basestring, type(None)))
2269
2270 def test_utf8_gecos(self):
2271 """Get user name/address from gecos.
2272
2273 :see: <https://bugs.launchpad.net/bzr/+bug/659763>.
2274 """
2275 self.requireFeature(features.pwd_module)
2276 import pwd
2277 fake_gecos = (
2278 '%s,,,,' % (fixtures.unicode_real_names[0])).encode('utf-8')
2279 uid = os.getuid()
2280 def fake_getpwuid(u):
2281 if u == uid:
2282 return type('FakeGecos', (),
2283 dict(pw_gecos=fake_gecos,
2284 pw_name=fixtures.user_names[0]))
2285 self.overrideAttr(pwd, 'getpwuid', fake_getpwuid)
2286 realname, username = config.get_user_name_from_os()
2287 self.assertEquals(fixtures.unicode_real_names[0], realname)
2288 self.assertEquals(fixtures.user_names[0], username)
22402289
=== 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 @@
39.. Fixes for situations where bzr would previously crash or give incorrect39.. Fixes for situations where bzr would previously crash or give incorrect
40 or undesirable results.40 or undesirable results.
4141
42* When reporting a crash without apport, don't print the full list of42* Bazaar now infers the default user email address on Unix from the local
43 plugins because it's often too long.43 account name plus the contents of ``/etc/mailname`` if that file exists.
44 (Martin Pool, #716389)44 In particular, this means that committing as root through etckeeper will
45 normally not require running ``bzr whoami`` first.
46 (Martin Pool, #616878)
4547
46* ``bzr push`` into a repository (that doesn't have a branch), will no48* ``bzr push`` into a repository (that doesn't have a branch), will no
47 longer copy all revisions in the repository. Only the ones in the49 longer copy all revisions in the repository. Only the ones in the
48 ancestry of the source branch, like it does in all other cases.50 ancestry of the source branch, like it does in all other cases.
49 (John Arbash Meinel, #465517)51 (John Arbash Meinel, #465517)
5052
53* Correctly handle user real names in ``/etc/passwd`` that contain
54 non-ascii characters; these are attempted to be interpreted as either
55 UTF-8 or the user's own encoding.
56 (Martin Pool, #659763)
57
51* Fix ``UnboundLocalError: local variable 'lock_url' in wait_lock`` error,58* Fix ``UnboundLocalError: local variable 'lock_url' in wait_lock`` error,
52 especially while trying to save configuration from QBzr.59 especially while trying to save configuration from QBzr.
53 (Martin Pool, #733136)60 (Martin Pool, #733136)
@@ -56,6 +63,10 @@
56 had changed. Bazaar was attempting to open and lock the master branch63 had changed. Bazaar was attempting to open and lock the master branch
57 twice in this case. (Andrew Bennetts, #733350)64 twice in this case. (Andrew Bennetts, #733350)
5865
66* When reporting a crash without apport, don't print the full list of
67 plugins because it's often too long.
68 (Martin Pool, #716389)
69
59Documentation70Documentation
60*************71*************
6172

Subscribers

People subscribed via source and target branches