Merge lp:~parthm/bzr/549310-mandatory-whoami into lp:bzr

[Parth Malwankar]

bzr does not guess whoami anymore. (parthm, #549310)

=== Fixed Bug #549310 ===

This proposal make setting username mandatory either using 'bzr whoami', EMAIL or BZR_EMAIL.
bzr does not try to guess username as login@host as thats not very reliable.

Behavior change is that init, init-repo, commit, whoami throw error suggesting the use of 'bzr whoami'.
Explicit checks are added to init and init-repo to ensure that username is avoid creation of branch/repo directory by failing early. For making tests work, EMAIL has be set to <email address hidden> in the test environment.

E.g.
sandbox@parthm-laptop:~$ /home/parthm/src/bzr.dev/549310-mandatory-whoami/bzr --no-plugins init-repo foo
bzr: ERROR: Unable to determine your name.
Please, set your name with the 'whoami' command.
E.g. bzr whoami "Your Name <email address hidden>"
sandbox@parthm-laptop:~$
sandbox@parthm-laptop:~$
sandbox@parthm-laptop:~$ /home/parthm/src/bzr.dev/549310-mandatory-whoami/bzr --no-plugins init foo
bzr: ERROR: Unable to determine your name.
Please, set your name with the 'whoami' command.
E.g. bzr whoami "Your Name <email address hidden>"
sandbox@parthm-laptop:~$
sandbox@parthm-laptop:~$ ls
examples.desktop tmp
sandbox@parthm-laptop:~$ /home/parthm/src/bzr.dev/549310-mandatory-whoami/bzr --no-plugins whoami
bzr: ERROR: Unable to determine your name.
Please, set your name with the 'whoami' command.
E.g. bzr whoami "Your Name <email address hidden>"
sandbox@parthm-laptop:~$

[Andrew Bennetts (spiv)]

It will be very nice to have this bug fixed.

However, I don't think we should require whoami to be set to be able to use init-repo or init. Especially for init-repo there are valid use cases for it that don't require an identity to be set, e.g. mirroring a set of branches.

review: Needs Fixing

[Parth Malwankar (parthm)]

> It will be very nice to have this bug fixed.
>
> However, I don't think we should require whoami to be set to be able to use
> init-repo or init. Especially for init-repo there are valid use cases for it
> that don't require an identity to be set, e.g. mirroring a set of branches.

I was myself curious as to where/why init/init-repo need whoami to
be set. This patch merely ensures that they fail early before creating
a directory on disk.

So, it seems this info is used by lockdir to write it to lock file
so bzr know who has locked the dir. I suppose it maybe better to
keep whoami as mandatory in such a situation. Thoughts?

Following are the interesting parts of the stack trace without the
fail early change.

Traceback (most recent call last):
  File "/storage/parth/src/bzr.dev/549310-mandatory-whoami/bzrlib/commands.py", line 907, in exception_to_return_code
    return the_callable(*args, **kwargs)
  File "/storage/parth/src/bzr.dev/549310-mandatory-whoami/bzrlib/commands.py", line 1111, in run_bzr
    ret = run(*run_argv)
  File "/storage/parth/src/bzr.dev/549310-mandatory-whoami/bzrlib/commands.py", line 685, in run_argv_aliases
    return self.run(**all_cmd_args)
  File "/storage/parth/src/bzr.dev/549310-mandatory-whoami/bzrlib/commands.py", line 700, in run
    return self._operation.run_simple(*args, **kwargs)
  File "/storage/parth/src/bzr.dev/549310-mandatory-whoami/bzrlib/cleanup.py", line 122, in run_simple
    self.cleanups, self.func, *args, **kwargs)
  File "/storage/parth/src/bzr.dev/549310-mandatory-whoami/bzrlib/cleanup.py", line 156, in _do_with_cleanups
    result = func(*args, **kwargs)
  File "/storage/parth/src/bzr.dev/549310-mandatory-whoami/bzrlib/builtins.py", line 1761, in run
    possible_transports=[to_transport])
  File "/storage/parth/src/bzr.dev/549310-mandatory-whoami/bzrlib/bzrdir.py", line 539, in create_branch_convenience
    bzrdir = BzrDir.create(base, format, possible_transports)
  File "/storage/parth/src/bzr.dev/549310-mandatory-whoami/bzrlib/bzrdir.py", line 315, in create
    return format.initialize_on_transport(t)
  File "/storage/parth/src/bzr.dev/549310-mandatory-whoami/bzrlib/bzrdir.py", line 1961, in initialize_on_transport
    return self._initialize_on_transport_vfs(transport)
  File "/storage/parth/src/bzr.dev/549310-mandatory-whoami/bzrlib/bzrdir.py", line 2098, in _initialize_on_transport_vfs
    control_files.lock_write()
  File "/storage/parth/src/bzr.dev/549310-mandatory-whoami/bzrlib/lockable_files.py", line 193, in lock_write
    token_from_lock = self._lock.lock_write(token=token)
  File "/storage/parth/src/bzr.dev/549310-mandatory-whoami/bzrlib/lockdir.py", line 600, in lock_write
    return self.wait_lock()
  File "/storage/parth/src/bzr.dev/549310-mandatory-whoami/bzrlib/lockdir.py", line 521, in wait_lock
    return self.attempt_lock()
  File "/storage/parth/src/bzr.dev/549310-mandatory-whoami/bzrlib/lockdir.py", line 482, in attempt_lock
    result = self._attempt_lock()
  File "/storage/parth/src/bzr.dev/549310-mandatory-whoami/bzrlib/lockdir.py", line 224, in _attempt_lock
    tmpname = self._create_pending_dir()
  File "/storage/parth/src/bzr.dev/549310-mandatory-whoami/bzr...

[Andrew Bennetts (spiv)]

Parth Malwankar wrote:
> > It will be very nice to have this bug fixed.
> >
> > However, I don't think we should require whoami to be set to be able to use
> > init-repo or init. Especially for init-repo there are valid use cases for it
> > that don't require an identity to be set, e.g. mirroring a set of branches.
>
> I was myself curious as to where/why init/init-repo need whoami to
> be set. This patch merely ensures that they fail early before creating
> a directory on disk.
>
> So, it seems this info is used by lockdir to write it to lock file
> so bzr know who has locked the dir. I suppose it maybe better to
> keep whoami as mandatory in such a situation. Thoughts?
>
> Following are the interesting parts of the stack trace without the
> fail early change.

Ah!

I think for lockdirs it doesn't matter so much what the name is. It's
not a name that will be recorded permanently in the revision history,
it's just an indication for a concurrent user of that repo/branch/tree
of who has got the lock.

Ideally I think that for lockdirs:

  * if the whoami is set, use that
  * if the whoami isn't set, then the best-guess of the old
    _auto_user_id code is good enough — that will almost certainly tell
    whoever is looking at the lock file enough to figure out who created
    the lock.

-Andrew.

[Martin Pool (mbp)]

On 28 April 2010 12:48, Andrew Bennetts <email address hidden> wrote:

> I think for lockdirs it doesn't matter so much what the name is.  It's
> not a name that will be recorded permanently in the revision history,
> it's just an indication for a concurrent user of that repo/branch/tree
> of who has got the lock.

+1

>
> Ideally I think that for lockdirs:
>
>  * if the whoami is set, use that
>  * if the whoami isn't set, then the best-guess of the old
>    _auto_user_id code is good enough — that will almost certainly tell
>    whoever is looking at the lock file enough to figure out who created
>    the lock.

even just putting the local username with no special tricks would be
enough. It's pretty transient.

--
Martin <http://launchpad.net/~mbp/>

[lp:~parthm/bzr/549310-mandatory-whoami]

5193. By Parth Malwankar on 2010-04-28

lockdir no long mandates whoami but uses unicode version of getuser

[Robert Collins (lifeless)]

Well, lockdirs are exposed over the network, so if we have any host
info that is better than nothing.

[Martin Pool (mbp)]

On 28 April 2010 16:45, Robert Collins <email address hidden> wrote:
> Well, lockdirs are exposed over the network, so if we have any host
> info that is better than nothing.

I'm pretty sure the process hostname is already included in a separate field.

--
Martin <http://launchpad.net/~mbp/>

[lp:~parthm/bzr/549310-mandatory-whoami]

5194. By Parth Malwankar on 2010-04-28

moved getuser_unicode to osutils.

5195. By Parth Malwankar on 2010-04-28

added tests for getuser_unicode

[Parth Malwankar (parthm)]

lockdir updated to use unicode version of getpass.getuser().
Added utility function osutils.getuser_unicode() for this.
init/init-repo now work even when whoami is not set.
Added errors.NoWhoami.

[Parth Malwankar (parthm)]

The review comments here are closed. Maybe someone can give this a second look.

[Martin Pool (mbp)]

That looks good to me. Perhaps you could add comments to the tests for init and init-repo explaining why they're even tested, because it's a bit surprising they would ever fail.

review: Approve

[lp:~parthm/bzr/549310-mandatory-whoami]

5196. By Parth Malwankar on 2010-05-05

added comment to init/init-repo pass tests for lacking whoami.

5197. By Parth Malwankar on 2010-05-05

merged in changes from trunk.

[Parth Malwankar (parthm)]

> That looks good to me. Perhaps you could add comments to the tests for init
> and init-repo explaining why they're even tested, because it's a bit
> surprising they would ever fail.

Done. Thanks for the review.

All open comments on this are closed, so if another reviewer could give it a look and there is nothing more to do on this patch I could try to land it.

[Martin Pool (mbp)]

I think this is sufficiently reviewed and you can land at your leisure.

--
Martin <http://launchpad.net/~mbp/>

=== modified file 'NEWS'
--- NEWS	2010-05-05 13:25:24 +0000
+++ NEWS	2010-05-05 14:13:28 +0000
@@ -13,6 +13,11 @@
 Compatibility Breaks
 ********************
 
+* ``bzr`` does not try to guess the username as ``username@hostname``
+  and requires it to be explictly set. This can be set using ``bzr
+  whoami``.
+  (Parth Malwankar, #549310)
+
 New Features
 ************
 

=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2010-04-24 09:08:27 +0000
+++ bzrlib/config.py	2010-05-05 14:13:28 +0000
@@ -257,8 +257,7 @@
 
         Something similar to 'Martin Pool <[e-mail]>'
 
-        $BZR_EMAIL can be set to override this (as well as the
-        deprecated $BZREMAIL), then
+        $BZR_EMAIL can be set to override this, then
         the concrete policy type is checked, and finally
         $EMAIL is examined.
         If none is found, a reasonable default is (hopefully)
@@ -278,11 +277,14 @@
         if v:
             return v.decode(osutils.get_user_encoding())
 
-        name, email = _auto_user_id()
-        if name:
-            return '%s <%s>' % (name, email)
-        else:
-            return email
+        raise errors.NoWhoami()
+
+    def ensure_username(self):
+        """Raise BzrCommandError if username is not set.
+
+        This method relies on the username() function raising the error.
+        """
+        self.username()
 
     def signature_checking(self):
         """What is the current policy for signature checking?."""
@@ -899,79 +901,6 @@
         return os.path.expanduser('~/.cache')
 
 
-def _auto_user_id():
-    """Calculate automatic user identification.
-
-    Returns (realname, email).
-
-    Only used when none is set in the environment or the id file.
-
-    This previously used the FQDN as the default domain, but that can
-    be very slow on machines where DNS is broken.  So now we simply
-    use the hostname.
-    """
-    import socket
-
-    if sys.platform == 'win32':
-        name = win32utils.get_user_name_unicode()
-        if name is None:
-            raise errors.BzrError("Cannot autodetect user name.\n"
-                                  "Please, set your name with command like:\n"
-                                  'bzr whoami "Your Name <[e-mail]>"')
-        host = win32utils.get_host_name_unicode()
-        if host is None:
-            host = socket.gethostname()
-        return name, (name + '@' + host)
-
-    try:
-        import pwd
-        uid = os.getuid()
-        try:
-            w = pwd.getpwuid(uid)
-        except KeyError:
-            raise errors.BzrCommandError('Unable to determine your name.  '
-                'Please use "bzr whoami" to set it.')
-
-        # 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:
-                encoding = osutils.get_user_encoding()
-                gecos = w.pw_gecos.decode(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:
-            realname = gecos
-        else:
-            realname = gecos[:comma]
-        if not realname:
-            realname = username
-
-    except ImportError:
-        import getpass
-        try:
-            user_encoding = osutils.get_user_encoding()
-            realname = username = getpass.getuser().decode(user_encoding)
-        except UnicodeDecodeError:
-            raise errors.BzrError("Can't decode username as %s." % \
-                    user_encoding)
-
-    return realname, (username + '@' + socket.gethostname())
-
-
 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/errors.py'
--- bzrlib/errors.py	2010-04-21 11:27:04 +0000
+++ bzrlib/errors.py	2010-05-05 14:13:28 +0000
@@ -3134,3 +3134,10 @@
     def __init__(self, bzrdir):
         self.bzrdir = bzrdir
 
+class NoWhoami(BzrError):
+
+    _fmt = ('Unable to determine your name.\n'
+        "Please, set your name with the 'whoami' command.\n"
+        'E.g. bzr whoami "Your Name <[e-mail]>"')
+
+

=== modified file 'bzrlib/lockdir.py'
--- bzrlib/lockdir.py	2010-03-03 22:59:21 +0000
+++ bzrlib/lockdir.py	2010-05-05 14:13:28 +0000
@@ -447,9 +447,9 @@
         # XXX: is creating this here inefficient?
         config = bzrlib.config.GlobalConfig()
         try:
-            user = config.user_email()
-        except errors.NoEmailInUsername:
             user = config.username()
+        except errors.NoWhoami:
+            user = osutils.getuser_unicode()
         s = rio.Stanza(hostname=get_host_name(),
                    pid=str(os.getpid()),
                    start_time=str(int(time.time())),

=== modified file 'bzrlib/osutils.py'
--- bzrlib/osutils.py	2010-05-03 06:25:46 +0000
+++ bzrlib/osutils.py	2010-05-05 14:13:28 +0000
@@ -26,6 +26,7 @@
 lazy_import(globals(), """
 from datetime import datetime
 import errno
+import getpass
 from ntpath import (abspath as _nt_abspath,
                     join as _nt_join,
                     normpath as _nt_normpath,
@@ -2302,3 +2303,15 @@
         return os.fdopen(os.open(filename, flags), mode, bufsize)
 else:
     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)
+    return username

=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py	2010-05-05 02:35:02 +0000
+++ bzrlib/tests/__init__.py	2010-05-05 14:13:28 +0000
@@ -1524,7 +1524,7 @@
             'EDITOR': None,
             'BZR_EMAIL': None,
             'BZREMAIL': None, # may still be present in the environment
-            'EMAIL': None,
+            'EMAIL': '[e-mail]', # set EMAIL as bzr does not guess
             'BZR_PROGRESS_BAR': None,
             'BZR_LOG': None,
             'BZR_PLUGIN_PATH': None,

=== modified file 'bzrlib/tests/blackbox/test_commit.py'
--- bzrlib/tests/blackbox/test_commit.py	2010-04-07 22:22:27 +0000
+++ bzrlib/tests/blackbox/test_commit.py	2010-05-05 14:13:28 +0000
@@ -710,3 +710,15 @@
         out, err = self.run_bzr_error(["empty commit message"],
             "commit tree/hello.txt", stdin="n\n")
         self.assertEqual(expected, tree.last_revision())
+
+    def test_commit_without_username(self):
+        """Ensure commit error if username is not set.
+        """
+        self.run_bzr(['init', 'foo'])
+        os.chdir('foo')
+        open('foo.txt', 'w').write('hello')
+        self.run_bzr(['add'])
+        osutils.set_or_unset_env('EMAIL', None)
+        osutils.set_or_unset_env('BZR_EMAIL', None)
+        out, err = self.run_bzr(['commit', '-m', 'initial'], 3)
+        self.assertContainsRe(err, 'Unable to determine your name')

=== modified file 'bzrlib/tests/blackbox/test_init.py'
--- bzrlib/tests/blackbox/test_init.py	2010-04-23 07:15:23 +0000
+++ bzrlib/tests/blackbox/test_init.py	2010-05-05 14:13:28 +0000
@@ -202,3 +202,17 @@
         self.assertEqual(True, branch._get_append_revisions_only())
         self.run_bzr_error(['cannot be set to append-revisions-only'],
                            'init --append-revisions-only --knit knit')
+
+    def test_init_without_username(self):
+        """Ensure init works if username is not set.
+        """
+        # bzr makes user specified whoami mandatory for operations
+        # like commit as whoami is recorded. init however is not so final
+        # and uses whoami only in a lock file. Without whoami the login name
+        # is used. This test is to ensure that init passes even when whoami
+        # is not available.
+        osutils.set_or_unset_env('EMAIL', None)
+        osutils.set_or_unset_env('BZR_EMAIL', None)
+        out, err = self.run_bzr(['init', 'foo'])
+        self.assertEqual(err, '')
+        self.assertTrue(os.path.exists('foo'))

=== modified file 'bzrlib/tests/blackbox/test_shared_repository.py'
--- bzrlib/tests/blackbox/test_shared_repository.py	2010-03-25 18:02:45 +0000
+++ bzrlib/tests/blackbox/test_shared_repository.py	2010-05-05 14:13:28 +0000
@@ -18,6 +18,7 @@
 
 import os
 
+from bzrlib import osutils
 from bzrlib.bzrdir import BzrDir, BzrDirMetaFormat1
 import bzrlib.errors as errors
 from bzrlib.tests import TestCaseInTempDir
@@ -145,3 +146,17 @@
         self.assertLength(0, calls)
         self.run_bzr("init-repository a")
         self.assertLength(1, calls)
+
+    def test_init_repo_without_username(self):
+        """Ensure init-repo works if username is not set.
+        """
+        # bzr makes user specified whoami mandatory for operations
+        # like commit as whoami is recorded. init-repo however is not so final
+        # and uses whoami only in a lock file. Without whoami the login name
+        # is used. This test is to ensure that init-repo passes even when whoami
+        # is not available.
+        osutils.set_or_unset_env('EMAIL', None)
+        osutils.set_or_unset_env('BZR_EMAIL', None)
+        out, err = self.run_bzr(['init-repo', 'foo'])
+        self.assertEqual(err, '')
+        self.assertTrue(os.path.exists('foo'))

=== modified file 'bzrlib/tests/blackbox/test_whoami.py'
--- bzrlib/tests/blackbox/test_whoami.py	2009-03-23 14:59:43 +0000
+++ bzrlib/tests/blackbox/test_whoami.py	2010-05-05 14:13:28 +0000
@@ -20,6 +20,7 @@
 import os
 
 import bzrlib
+from bzrlib import osutils
 from bzrlib.branch import Branch
 from bzrlib.tests.blackbox import ExternalBase
 
@@ -111,3 +112,11 @@
         self.assertEquals('"Branch Identity" does not seem to contain an '
                           'email address.  This is allowed, but not '
                           'recommended.\n', display)
+
+    def test_whoami_not_set(self):
+        """Ensure whoami error if username is not set.
+        """
+        osutils.set_or_unset_env('EMAIL', None)
+        osutils.set_or_unset_env('BZR_EMAIL', None)
+        out, err = self.run_bzr(['whoami'], 3)
+        self.assertContainsRe(err, 'Unable to determine your name')

=== modified file 'bzrlib/tests/test_osutils.py'
--- bzrlib/tests/test_osutils.py	2010-04-27 07:52:08 +0000
+++ bzrlib/tests/test_osutils.py	2010-05-05 14:13:28 +0000
@@ -2018,3 +2018,14 @@
         self.assertEquals(self.path, 'test_file')
         self.assertEquals(self.uid, s.st_uid)
         self.assertEquals(self.gid, s.st_gid)
+
+class TestGetuserUnicode(tests.TestCase):
+
+    def test_ascii_user(self):
+        osutils.set_or_unset_env('LOGNAME', 'jrandom')
+        self.assertEqual(u'jrandom', osutils.getuser_unicode())
+
+    def test_unicode_user(self):
+        ue = osutils.get_user_encoding()
+        osutils.set_or_unset_env('LOGNAME', u'jrandom\xb6'.encode(ue))
+        self.assertEqual(u'jrandom\xb6', osutils.getuser_unicode())