[PATCH] bzr whoami
John Arbash Meinel
john at arbash-meinel.com
Sun Jul 2 03:43:27 BST 2006
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Robey Pointer wrote:
>
...
>> Could you please add a note to NEWS, and please update the user
>> documentation to explain that the userid can be set this way.
>
> Okay, I edited NEWS, and a couple of relevant files in doc/ (including
> the tutorial). I edited the "how to set email" wiki page, but since the
> tutorial wiki page seems to be a direct copy of the bzr source, I only
> edited the source.
>
> The bundle is pretty big now, so I'm only attaching the diff. The
> branch still lives here:
> http://www.lag.net/~robey/code/bzr.dev.whoami
>
> robey
>
In all the discussion about registering branches, it seems we forgot to
review your patch. :)
>
> === added file bzrlib/tests/blackbox/test_whoami.py // file-id:test_whoami.py-2
> ... 0060629025641-8h3m2ch7kutqx7ug-1 // last-changed:robey at lag.net-200606292034
> ... 39-d32a68c74428c9db
> --- /dev/null
> +++ bzrlib/tests/blackbox/test_whoami.py
> @@ -0,0 +1,97 @@
> +# Copyright (C) 2005 by Canonical Ltd
> +# -*- coding: utf-8 -*-
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> +
^-- Three comments on the copyright statement.
1) it needs 2006 rather than 2005 (or maybe 2005,2006)
2) We've decided to use # along the whole left hand side, no gaps
# Copyright
#
# This program
...
3) I believe we only include the -*- coding -*- line in files that
actually need it. And I didn't see any unescaped utf-8. (Which we
generally prefer anyway).
> +
> +"""Black-box tests for bzr whoami.
> +"""
> +
Single line doc strings have their closing quotes on the same line:
"""Black-box tests for bzr whoami."""
> +import os
> +
> +import bzrlib
> +from bzrlib.branch import Branch
> +from bzrlib.tests.blackbox import ExternalBase
> +
> +
> +class TestWhoami(ExternalBase):
> +
> + def test_whoami(self):
> + # this should always identify something, if only "john at localhost"
> + out = self.run_bzr("whoami")[0]
> + self.assertTrue(len(out) > 0)
> + self.assertEquals(out.count('@'), 1)
We generally try to put the expected value first. (We haven't been 100%
consistent on this, but we are trying).
So:
self.assertEqual(1, out.count('@'))
(I prefer assertEqual, but assertEquals is valid)
> +
> + out = self.run_bzr("whoami", "--email")[0]
> + self.assertTrue(len(out) > 0)
> + self.assertEquals(out.count('@'), 1)
> +
> + def test_whoami_branch(self):
> + """branch specific user identity works."""
> + self.run_bzr('init')
> + b = bzrlib.branch.Branch.open('.')
> + b.get_config().set_user_option('email', 'Branch Identity <branch at identi.ty>')
> + bzr_email = os.environ.get('BZREMAIL')
> + if bzr_email is not None:
> + del os.environ['BZREMAIL']
> + try:
> + whoami = self.run_bzr("whoami")[0]
> + whoami_email = self.run_bzr("whoami", "--email")[0]
> + self.assertTrue(whoami.startswith('Branch Identity <branch at identi.ty>'))
> + self.assertTrue(whoami_email.startswith('branch at identi.ty'))
> +
> + # Verify that the environment variable overrides the value
> + # in the file
> + os.environ['BZREMAIL'] = 'Different ID <other at environ.ment>'
> + whoami = self.run_bzr("whoami")[0]
> + whoami_email = self.run_bzr("whoami", "--email")[0]
> + self.assertTrue(whoami.startswith('Different ID <other at environ.ment>'))
> + self.assertTrue(whoami_email.startswith('other at environ.ment'))
> + finally:
> + if bzr_email is not None:
> + os.environ['BZREMAIL'] = bzr_email
> +
> + def test_whoami_utf8(self):
> + """verify that an identity can be in utf-8."""
> + self.run_bzr('init')
> + self.run_bzr('whoami', u'Branch Identity \u20ac <branch at identi.ty>', encoding='utf-8')
You only need to set the 'encoding' parameter if you want to force the
sys.stdout() encoding.
So one thing we need to decide is if 'whoami' should fail if it cannot
print your name properly in the current encoding. I'm a little torn. On
one hand I would like to be strict, because a shell front end might use
whomai.
On the other, it might confuse users if they get an encoding error while
trying to just check if they set their username correctly.
> + bzr_email = os.environ.get('BZREMAIL')
> + if bzr_email is not None:
> + del os.environ['BZREMAIL']
> + try:
> + whoami = self.run_bzr("whoami", encoding='utf-8')[0]
> + whoami_email = self.run_bzr("whoami", "--email", encoding='utf-8')[0]
> + self.assertTrue(whoami.startswith('Branch Identity \xe2\x82\xac <branch at identi.ty>'))
> + self.assertTrue(whoami_email.startswith('branch at identi.ty'))
> + finally:
> + if bzr_email is not None:
> + os.environ['BZREMAIL'] = bzr_email
> +
We have 'assertStartsWith' which gives a better error message if the
string doesn't start correctly.
Also, I think you can just do:
self.assertEqual('Branch Identity \xe2\x82\xac <branch at identity>\n',
whoami)
Since there should only be one blank line at the end.
I would also tend to do:
whoami = self.run_bzr("whoami", encoding='utf-8')[0]
self.assertEqual(..., whoami)
whoami_email = self.run_bzr("whoami", "--email", encoding='utf-8')[0]
self.assertEqual(..., whoami_email)
Rather than calling run_bzr twice, and then checking the output twice.
> + def test_whoami_ascii(self):
> + """verify that whoami doesn't totally break when in utf-8, using an ascii encoding."""
v- in general, I prefer 'run_bzr' to runbzr. I personally think runbzr
should go away. (It only exists on ExternalBase, not TestCaseInTempDir).
Also, it is a little cleaner to use:
wt = self.make_branch_and_tree('.')
Rather than run_bzr('init').
make_branch_and_tree just creates a branch, rather than running through
all the command processing overhead (and associated entries in the logfile)
> + self.runbzr('init')
> + b = bzrlib.branch.Branch.open('.')
> + b.get_config().set_user_option('email', u'Branch Identity \u20ac <branch at identi.ty>')
> + bzr_email = os.environ.get('BZREMAIL')
> + if bzr_email is not None:
> + del os.environ['BZREMAIL']
> + try:
> + whoami = self.run_bzr("whoami", encoding='ascii')[0]
> + whoami_email = self.run_bzr("whoami", "--email", encoding='ascii')[0]
> + self.assertTrue(whoami.startswith('Branch Identity ? <branch at identi.ty>'))
> + self.assertTrue(whoami_email.startswith('branch at identi.ty'))
> + finally:
> + if bzr_email is not None:
> + os.environ['BZREMAIL'] = bzr_email
...
> @display_command
> - def run(self, email=False):
> - try:
> + def run(self, email=False, branch=False, name=None):
> + if name is None:
> + # use branch if we're inside one; otherwise global config
> + try:
> + c = WorkingTree.open_containing(u'.')[0].branch.get_config()
> + except NotBranchError:
> + c = config.GlobalConfig()
> + if email:
> + self.outf.write(c.user_email())
> + else:
> + self.outf.write(c.username())
> + self.outf.write('\n')
> + return
> +
> + # use global config unless --branch given
> + if branch:
> c = WorkingTree.open_containing(u'.')[0].branch.get_config()
> - except NotBranchError:
> + else:
> c = config.GlobalConfig()
> - if email:
> - print c.user_email()
> - else:
> - print c.username()
> + c.set_user_option('email', name)
>
Something doesn't seem right here. I really don't like the look of
WorkingTree.open_containing('.')[0].branch.get_config()
Also, because whoami is a branch config, it should still work even
without a working tree. So I would rather see:
if name is None:
try:
branch = Branch.open_containing('.')
except NotBranchError:
c = config.GlobalConfig()
else:
c = branch.get_config()
It doesn't really matter here, but it turns out it is significantly
faster to call:
self.outf.write(text + '\n')
rather than
self.outf.write(text)
self.outf.write('\n')
I think we may want to validate that the supplied value really is a:
"Full Name <email at address.com>"
Rather than just letting them do:
bzr whoami george
We do accept short names (I don't think we explicitly require email
addresses anywhere). But we would probably at least want to warn the
user that it is recommended to use a full email address, etc.
>
> class cmd_nick(Command):
>
> === modified file bzrlib/config.py // last-changed:robey at lag.net-20060629051253
> ... -f3c6c1306aebcb3d
> --- bzrlib/config.py
> +++ bzrlib/config.py
> @@ -337,6 +337,19 @@
> def __init__(self):
> super(GlobalConfig, self).__init__(config_filename)
>
> + def set_user_option(self, option, value):
> + """Save option and its value in the configuration."""
> + # FIXME: RBC 20051029 This should refresh the parser and also take a
> + # file lock on bazaar.conf.
> + conf_dir = os.path.dirname(self._get_filename())
> + ensure_config_dir_exists(conf_dir)
> + if 'DEFAULT' not in self._get_parser():
> + self._get_parser()['DEFAULT'] = {}
> + self._get_parser()['DEFAULT'][option] = value
> + f = open(self._get_filename(), 'wb')
> + self._get_parser().write(f)
> + f.close()
> +
>
> class LocationConfig(IniBasedConfig):
> """A configuration object that gives the policy for a location."""
> @@ -597,13 +610,25 @@
> uid = os.getuid()
> w = pwd.getpwuid(uid)
>
> - try:
> - gecos = w.pw_gecos.decode(bzrlib.user_encoding)
> - username = w.pw_name.decode(bzrlib.user_encoding)
> - except UnicodeDecodeError:
> - # We're using pwd, therefore we're on Unix, so /etc/passwd is ok.
> - raise errors.BzrError("Can't decode username in " \
> - "/etc/passwd as %s." % bzrlib.user_encoding)
> + # 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.)
> + try:
> + gecos = w.pw_gecos.decode('utf-8')
> + encoding = 'utf-8'
> + except UnicodeError:
> + try:
> + gecos = w.pw_gecos.decode(bzrlib.user_encoding)
> + encoding = bzrlib.user_encoding
> + except UnicodeError:
> + raise errors.BzrCommandError('Unable to determine your name. '
> + 'Use "bzr whoami" to set it.')
> + try:
> + username = w.pw_name.decode(encoding)
> + except UnicodeError:
> + raise errors.BzrCommandError('Unable to determine your name. '
> + 'Use "bzr whoami" to set it.')
>
> comma = gecos.find(',')
> if comma == -1:
^-- I wish there was a good way to write a test for this. But pwd is an
extension module that seems hooked into PAM, and I *really* don't think
we want to try to hijack pam.
All the other changes look good. Including the tutorial fixes.
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFEpzLPJdeBCYSNAAMRAg4NAKDXL3Sze93/iHXYp61MFjUyr+VWFwCfXQzA
B8AHrVx4d60sIWJ6Rm89+aA=
=w2/6
-----END PGP SIGNATURE-----
More information about the bazaar
mailing list