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

Proposed by Martin Pool on 2011-04-01
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 2011-04-01 Approve on 2011-04-01
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.
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
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

Andrew Bennetts (spiv) wrote :

sent to pqm by email

Jelmer Vernooij (jelmer) wrote :

sent to pqm by email

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 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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/config.py'
2--- bzrlib/config.py 2011-01-12 18:12:58 +0000
3+++ bzrlib/config.py 2011-04-01 04:04:23 +0000
4@@ -63,6 +63,7 @@
5 """
6
7 import os
8+import string
9 import sys
10
11 from bzrlib import commands
12@@ -271,21 +272,21 @@
13 the concrete policy type is checked, and finally
14 $EMAIL is examined.
15 If no username can be found, errors.NoWhoami exception is raised.
16-
17- TODO: Check it's reasonably well-formed.
18 """
19 v = os.environ.get('BZR_EMAIL')
20 if v:
21 return v.decode(osutils.get_user_encoding())
22-
23 v = self._get_user_id()
24 if v:
25 return v
26-
27 v = os.environ.get('EMAIL')
28 if v:
29 return v.decode(osutils.get_user_encoding())
30-
31+ name, email = _auto_user_id()
32+ if name and email:
33+ return '%s <%s>' % (name, email)
34+ elif email:
35+ return email
36 raise errors.NoWhoami()
37
38 def ensure_username(self):
39@@ -1213,6 +1214,92 @@
40 return os.path.expanduser('~/.cache')
41
42
43+def _get_default_mail_domain():
44+ """If possible, return the assumed default email domain.
45+
46+ :returns: string mail domain, or None.
47+ """
48+ if sys.platform == 'win32':
49+ # No implementation yet; patches welcome
50+ return None
51+ try:
52+ f = open('/etc/mailname')
53+ except (IOError, OSError), e:
54+ return None
55+ try:
56+ domain = f.read().strip()
57+ return domain
58+ finally:
59+ f.close()
60+
61+
62+def _auto_user_id():
63+ """Calculate automatic user identification.
64+
65+ :returns: (realname, email), either of which may be None if they can't be
66+ determined.
67+
68+ Only used when none is set in the environment or the id file.
69+
70+ This only returns an email address if we can be fairly sure the
71+ address is reasonable, ie if /etc/mailname is set on unix.
72+
73+ This doesn't use the FQDN as the default domain because that may be
74+ slow, and it doesn't use the hostname alone because that's not normally
75+ a reasonable address.
76+ """
77+ if sys.platform == 'win32':
78+ # No implementation to reliably determine Windows default mail
79+ # address; please add one.
80+ return None, None
81+
82+ default_mail_domain = _get_default_mail_domain()
83+ if not default_mail_domain:
84+ return None, None
85+ realname, username = get_user_name_from_os()
86+ return realname, (username + '@' + default_mail_domain)
87+
88+
89+def get_user_name_from_os():
90+ """Get information about the user from /etc/passwd or similar.
91+
92+ Used if no identity is defined.
93+
94+ :returns: (realname, local_username) or (None, None) if they can't be
95+ determined.
96+ """
97+ if sys.platform == 'win32':
98+ # win32 implementation welcome.
99+ return None, None
100+
101+ import pwd
102+ uid = os.getuid()
103+ try:
104+ w = pwd.getpwuid(uid)
105+ except KeyError:
106+ trace.mutter('no passwd entry for uid %d?' % uid)
107+ return None, None
108+
109+ # we try utf-8 first, because on many variants (like Linux),
110+ # /etc/passwd "should" be in utf-8, and because it's unlikely to give
111+ # false positives. (many users will have their user encoding set to
112+ # latin-1, which cannot raise UnicodeError.)
113+ for encoding in ['utf-8', osutils.get_user_encoding()]:
114+ try:
115+ gecos = w.pw_gecos.decode(encoding)
116+ username = w.pw_name.decode(encoding)
117+ except UnicodeError, e:
118+ trace.mutter("cannot decode passwd entry %r as %r" %
119+ ((w.pw_name, w.pw_gecos,), encoding))
120+ continue
121+ else:
122+ break
123+ else:
124+ return None, None
125+ realname = gecos.split(',')[0]
126+ return realname, username
127+
128+
129 def parse_username(username):
130 """Parse e-mail username and return a (name, address) tuple."""
131 match = re.match(r'(.*?)\s*<?([\w+.-]+@[\w+.-]+)>?', username)
132
133=== modified file 'bzrlib/tests/features.py'
134--- bzrlib/tests/features.py 2011-01-12 01:01:53 +0000
135+++ bzrlib/tests/features.py 2011-04-01 04:04:23 +0000
136@@ -45,6 +45,7 @@
137 pywintypes = tests.ModuleAvailableFeature('pywintypes')
138 sphinx = tests.ModuleAvailableFeature('sphinx')
139 subunit = tests.ModuleAvailableFeature('subunit')
140+pwd_module = tests.ModuleAvailableFeature('pwd')
141
142
143 class _BackslashDirSeparatorFeature(tests.Feature):
144
145=== modified file 'bzrlib/tests/fixtures.py'
146--- bzrlib/tests/fixtures.py 2010-06-26 01:07:16 +0000
147+++ bzrlib/tests/fixtures.py 2011-04-01 04:04:23 +0000
148@@ -97,3 +97,15 @@
149 def __exit__(self, exc_type, exc_val, exc_tb):
150 self._calls.append('__exit__')
151 return False # propogate exceptions.
152+
153+
154+unicode_real_names = [
155+ u'joe user \u2460',
156+ u'joe user \u2461',
157+ ]
158+
159+"""local-part of user email addressess"""
160+user_names = [
161+ 'user1',
162+ 'user2',
163+ ]
164
165=== modified file 'bzrlib/tests/test_config.py'
166--- bzrlib/tests/test_config.py 2011-02-22 09:49:45 +0000
167+++ bzrlib/tests/test_config.py 2011-04-01 04:04:23 +0000
168@@ -41,6 +41,8 @@
169 )
170 from bzrlib.tests import (
171 features,
172+ fixtures,
173+ TestSkipped,
174 scenarios,
175 )
176 from bzrlib.util.configobj import configobj
177@@ -2237,3 +2239,50 @@
178 # test_user_prompted ?
179 class TestAuthenticationRing(tests.TestCaseWithTransport):
180 pass
181+
182+
183+class TestAutoUserId(tests.TestCase):
184+ """Test inferring an automatic user name."""
185+
186+ def test_auto_user_id(self):
187+ """Automatic inference of user name.
188+
189+ This is a bit hard to test in an isolated way, because it depends on
190+ system functions that go direct to /etc or perhaps somewhere else.
191+ But it's reasonable to say that on Unix, with an /etc/mailname, we ought
192+ to be able to choose a user name with no configuration.
193+ """
194+ if sys.platform == 'win32':
195+ raise TestSkipped("User name inference not implemented on win32")
196+ realname, address = config._auto_user_id()
197+ if os.path.exists('/etc/mailname'):
198+ self.assertTrue(realname)
199+ self.assertTrue(address)
200+ else:
201+ self.assertEquals((None, None), (realname, address))
202+
203+ def test_get_user_name_from_os_smoke(self):
204+ """get_user_name_from_pwd may not work but it must not crash."""
205+ realname, local_username = config.get_user_name_from_os()
206+ self.assertIsInstance(realname, (basestring, type(None)))
207+ self.assertIsInstance(local_username, (basestring, type(None)))
208+
209+ def test_utf8_gecos(self):
210+ """Get user name/address from gecos.
211+
212+ :see: <https://bugs.launchpad.net/bzr/+bug/659763>.
213+ """
214+ self.requireFeature(features.pwd_module)
215+ import pwd
216+ fake_gecos = (
217+ '%s,,,,' % (fixtures.unicode_real_names[0])).encode('utf-8')
218+ uid = os.getuid()
219+ def fake_getpwuid(u):
220+ if u == uid:
221+ return type('FakeGecos', (),
222+ dict(pw_gecos=fake_gecos,
223+ pw_name=fixtures.user_names[0]))
224+ self.overrideAttr(pwd, 'getpwuid', fake_getpwuid)
225+ realname, username = config.get_user_name_from_os()
226+ self.assertEquals(fixtures.unicode_real_names[0], realname)
227+ self.assertEquals(fixtures.user_names[0], username)
228
229=== modified file 'doc/en/release-notes/bzr-2.3.txt'
230--- doc/en/release-notes/bzr-2.3.txt 2011-04-01 01:37:43 +0000
231+++ doc/en/release-notes/bzr-2.3.txt 2011-04-01 04:04:23 +0000
232@@ -39,15 +39,22 @@
233 .. Fixes for situations where bzr would previously crash or give incorrect
234 or undesirable results.
235
236-* When reporting a crash without apport, don't print the full list of
237- plugins because it's often too long.
238- (Martin Pool, #716389)
239+* Bazaar now infers the default user email address on Unix from the local
240+ account name plus the contents of ``/etc/mailname`` if that file exists.
241+ In particular, this means that committing as root through etckeeper will
242+ normally not require running ``bzr whoami`` first.
243+ (Martin Pool, #616878)
244
245 * ``bzr push`` into a repository (that doesn't have a branch), will no
246 longer copy all revisions in the repository. Only the ones in the
247 ancestry of the source branch, like it does in all other cases.
248 (John Arbash Meinel, #465517)
249
250+* Correctly handle user real names in ``/etc/passwd`` that contain
251+ non-ascii characters; these are attempted to be interpreted as either
252+ UTF-8 or the user's own encoding.
253+ (Martin Pool, #659763)
254+
255 * Fix ``UnboundLocalError: local variable 'lock_url' in wait_lock`` error,
256 especially while trying to save configuration from QBzr.
257 (Martin Pool, #733136)
258@@ -56,6 +63,10 @@
259 had changed. Bazaar was attempting to open and lock the master branch
260 twice in this case. (Andrew Bennetts, #733350)
261
262+* When reporting a crash without apport, don't print the full list of
263+ plugins because it's often too long.
264+ (Martin Pool, #716389)
265+
266 Documentation
267 *************
268

Subscribers

People subscribed via source and target branches