Raise exception when user identity misses email. Fixes #602

NEWS

@@ -9,6 +9,9 @@
   * Allow comment characters (#, ;) within configuration file strings
     (Daniel Andersson, #579)
 
+  * Raise exception when passing in invalid author/committer values
+    to Repo.do_commit(). (Jelmer Vernooij, #602)
+
  IMPROVEMENTS
 
   * Add a fastimport ``extra``. (Jelmer Vernooij)

dulwich/repo.py

@@ -107,6 +107,27 @@ BASE_DIRECTORIES = [
 DEFAULT_REF = b'refs/heads/master'
 
 
+class InvalidUserIdentity(Exception):
+    """User identity is not of the format 'user <email>'"""
+
+    def __init__(self, identity):
+        self.identity = identity
+
+
+def check_user_identity(identity):
+    """Verify that a user identity is formatted correctly.
+
+    :param identity: User identity bytestring
+    :raise InvalidUserIdentity: Raised when identity is invalid
+    """
+    try:
+        fst, snd = identity.split(b' <', 1)
+    except ValueError:
+        raise InvalidUserIdentity(identity)
+    if not b'>' in snd:
+        raise InvalidUserIdentity(identity)
+
+
 def parse_graftpoints(graftpoints):
     """Convert a list of graftpoints into a dict
 
@@ -606,6 +627,7 @@ class BaseRepo(object):
             merge_heads = []
         if committer is None:
             committer = self._get_user_identity()
+        check_user_identity(committer)
         c.committer = committer
         if commit_timestamp is None:
             # FIXME: Support GIT_COMMITTER_DATE environment variable
@@ -620,6 +642,7 @@ class BaseRepo(object):
             # variables
             author = committer
         c.author = author
+        check_user_identity(author)
         if author_timestamp is None:
             # FIXME: Support GIT_AUTHOR_DATE environment variable
             author_timestamp = commit_timestamp
@@ -764,6 +787,7 @@ class Repo(BaseRepo):
                 raise
         if committer is None:
             committer = self._get_user_identity()
+        check_user_identity(committer)
         if timestamp is None:
             timestamp = int(time.time())
         if timezone is None:

dulwich/tests/test_index.py

@@ -587,7 +587,8 @@ class GetUnstagedChangesTests(TestCase):
                 f.write(b'origstuff')
 
             repo.stage(['foo1', 'foo2'])
-            repo.do_commit(b'test status', author=b'', committer=b'')
+            repo.do_commit(b'test status', author=b'author <email>',
+                           committer=b'committer <email>')
 
             with open(foo1_fullpath, 'wb') as f:
                 f.write(b'newstuff')
@@ -612,7 +613,8 @@ class GetUnstagedChangesTests(TestCase):
                 f.write(b'origstuff')
 
             repo.stage(['foo1'])
-            repo.do_commit(b'test status', author=b'', committer=b'')
+            repo.do_commit(b'test status', author=b'author <email>',
+                           committer=b'committer <email>')
 
             os.unlink(foo1_fullpath)
 

dulwich/tests/test_porcelain.py

@@ -235,7 +235,7 @@ class AddTests(PorcelainTestCase):
             f.write("\n")
         porcelain.add(repo=self.repo.path, paths=[fullpath])
         porcelain.commit(repo=self.repo.path, message=b'test',
-                         author=b'test', committer=b'test')
+                         author=b'test <email>', committer=b'test <email>')
 
         # Add a second test file and a file in a directory
         with open(os.path.join(self.repo.path, 'foo'), 'w') as f:
@@ -266,7 +266,8 @@ class AddTests(PorcelainTestCase):
             os.chdir(os.path.join(self.repo.path, 'foo'))
             porcelain.add(repo=self.repo.path)
             porcelain.commit(repo=self.repo.path, message=b'test',
-                             author=b'test', committer=b'test')
+                             author=b'test <email>',
+                             committer=b'test <email>')
         finally:
             os.chdir(cwd)
 
@@ -322,8 +323,9 @@ class RemoveTests(PorcelainTestCase):
         with open(fullpath, 'w') as f:
             f.write("BAR")
         porcelain.add(self.repo.path, paths=[fullpath])
-        porcelain.commit(repo=self.repo, message=b'test', author=b'test',
-                         committer=b'test')
+        porcelain.commit(repo=self.repo, message=b'test',
+                         author=b'test <email>',
+                         committer=b'test <email>')
         self.assertTrue(os.path.exists(os.path.join(self.repo.path, 'foo')))
         cwd = os.getcwd()
         try:
@@ -657,7 +659,8 @@ class PushTests(PorcelainTestCase):
         errstream = BytesIO()
 
         porcelain.commit(repo=self.repo.path, message=b'init',
-                         author=b'', committer=b'')
+                         author=b'author <email>',
+                         committer=b'committer <email>')
 
         # Setup target repo cloned from temp test repo
         clone_path = tempfile.mkdtemp()
@@ -674,7 +677,8 @@ class PushTests(PorcelainTestCase):
         os.close(handle)
         porcelain.add(repo=clone_path, paths=[fullpath])
         porcelain.commit(repo=clone_path, message=b'push',
-                         author=b'', committer=b'')
+                         author=b'author <email>',
+                         committer=b'committer <email>')
 
         # Setup a non-checked out branch in the remote
         refs_path = b"refs/heads/foo"
@@ -709,7 +713,8 @@ class PushTests(PorcelainTestCase):
         errstream = BytesIO()
 
         porcelain.commit(repo=self.repo.path, message=b'init',
-                         author=b'', committer=b'')
+                         author=b'author <email>',
+                         committer=b'committer <email>')
 
         # Setup target repo cloned from temp test repo
         clone_path = tempfile.mkdtemp()
@@ -743,7 +748,8 @@ class PullTests(PorcelainTestCase):
         os.close(handle)
         porcelain.add(repo=self.repo.path, paths=fullpath)
         porcelain.commit(repo=self.repo.path, message=b'test',
-                         author=b'test', committer=b'test')
+                         author=b'test <email>',
+                         committer=b'test <email>')
 
         # Setup target repo
         self.target_path = tempfile.mkdtemp()
@@ -757,7 +763,8 @@ class PullTests(PorcelainTestCase):
         os.close(handle)
         porcelain.add(repo=self.repo.path, paths=fullpath)
         porcelain.commit(repo=self.repo.path, message=b'test2',
-                         author=b'test2', committer=b'test2')
+                         author=b'test2 <email>',
+                         committer=b'test2 <email>')
 
         self.assertTrue(b'refs/heads/master' in self.repo.refs)
         self.assertTrue(b'refs/heads/master' in target_repo.refs)
@@ -806,7 +813,8 @@ class StatusTests(PorcelainTestCase):
 
         porcelain.add(repo=self.repo.path, paths=[fullpath])
         porcelain.commit(repo=self.repo.path, message=b'test status',
-                         author=b'', committer=b'')
+                         author=b'author <email>',
+                         committer=b'committer <email>')
 
         # modify access and modify time of path
         os.utime(fullpath, (0, 0))
@@ -837,7 +845,8 @@ class StatusTests(PorcelainTestCase):
             f.write('stuff')
         porcelain.add(repo=self.repo.path, paths=fullpath)
         porcelain.commit(repo=self.repo.path, message=b'test status',
-                         author=b'', committer=b'')
+                         author=b'author <email>',
+                         committer=b'committer <email>')
 
         filename = 'foo'
         fullpath = os.path.join(self.repo.path, filename)
@@ -861,7 +870,8 @@ class StatusTests(PorcelainTestCase):
             f.write('stuff')
         porcelain.add(repo=self.repo.path, paths=fullpath)
         porcelain.commit(repo=self.repo.path, message=b'test status',
-                         author=b'', committer=b'')
+                         author=b'author <email>',
+                         committer=b'committer <email>')
         with open(fullpath, 'w') as f:
             f.write('otherstuff')
         porcelain.add(repo=self.repo.path, paths=fullpath)
@@ -882,7 +892,8 @@ class StatusTests(PorcelainTestCase):
             f.write('stuff')
         porcelain.add(repo=self.repo.path, paths=fullpath)
         porcelain.commit(repo=self.repo.path, message=b'test status',
-                         author=b'', committer=b'')
+                         author=b'author <email>',
+                         committer=b'committer <email>')
         cwd = os.getcwd()
         try:
             os.chdir(self.repo.path)
@@ -955,7 +966,8 @@ class ReceivePackTests(PorcelainTestCase):
             f.write('stuff')
         porcelain.add(repo=self.repo.path, paths=fullpath)
         self.repo.do_commit(message=b'test status',
-                            author=b'', committer=b'',
+                            author=b'author <email>',
+                            committer=b'committer <email>',
                             author_timestamp=1402354300,
                             commit_timestamp=1402354300, author_timezone=0,
                             commit_timezone=0)
@@ -964,10 +976,10 @@ class ReceivePackTests(PorcelainTestCase):
                 self.repo.path, BytesIO(b"0000"), outf)
         outlines = outf.getvalue().splitlines()
         self.assertEqual([
-            b'00919e65bdcf4a22cdd4f3700604a275cd2aaf146b23 HEAD\x00 report-status '  # noqa: E501
+            b'0091319b56ce3aee2d489f759736a79cc552c9bb86d9 HEAD\x00 report-status '  # noqa: E501
             b'delete-refs quiet ofs-delta side-band-64k '
             b'no-done symref=HEAD:refs/heads/master',
-            b'003f9e65bdcf4a22cdd4f3700604a275cd2aaf146b23 refs/heads/master',
+           b'003f319b56ce3aee2d489f759736a79cc552c9bb86d9 refs/heads/master',
             b'0000'], outlines)
         self.assertEqual(0, exitcode)
 
@@ -1026,7 +1038,8 @@ class FetchTests(PorcelainTestCase):
         os.close(handle)
         porcelain.add(repo=self.repo.path, paths=fullpath)
         porcelain.commit(repo=self.repo.path, message=b'test',
-                         author=b'test', committer=b'test')
+                         author=b'test <email>',
+                         committer=b'test <email>')
 
         # Setup target repo
         target_path = tempfile.mkdtemp()
@@ -1039,7 +1052,8 @@ class FetchTests(PorcelainTestCase):
         os.close(handle)
         porcelain.add(repo=self.repo.path, paths=fullpath)
         porcelain.commit(repo=self.repo.path, message=b'test2',
-                         author=b'test2', committer=b'test2')
+                         author=b'test2 <email>',
+                         committer=b'test2 <email>')
 
         self.assertFalse(self.repo[b'HEAD'].id in target_repo)
         target_repo.close()
@@ -1069,7 +1083,8 @@ class LsTreeTests(PorcelainTestCase):
 
     def test_empty(self):
         porcelain.commit(repo=self.repo.path, message=b'test status',
-                         author=b'', committer=b'')
+                         author=b'author <email>',
+                         committer=b'committer <email>')
 
         f = StringIO()
         porcelain.ls_tree(self.repo, b"HEAD", outstream=f)
@@ -1083,7 +1098,8 @@ class LsTreeTests(PorcelainTestCase):
 
         porcelain.add(repo=self.repo.path, paths=[fullpath])
         porcelain.commit(repo=self.repo.path, message=b'test status',
-                         author=b'', committer=b'')
+                         author=b'author <email>',
+                         committer=b'committer <email>')
 
         f = StringIO()
         porcelain.ls_tree(self.repo, b"HEAD", outstream=f)
@@ -1099,7 +1115,8 @@ class LsRemoteTests(PorcelainTestCase):
 
     def test_some(self):
         cid = porcelain.commit(repo=self.repo.path, message=b'test status',
-                               author=b'', committer=b'')
+                               author=b'author <email>',
+                               committer=b'committer <email>')
 
         self.assertEqual({
             b'refs/heads/master': cid,

dulwich/tests/test_repository.py

@@ -37,8 +37,10 @@ from dulwich import objects
 from dulwich.config import Config
 from dulwich.errors import NotGitRepository
 from dulwich.repo import (
+    InvalidUserIdentity,
     Repo,
     MemoryRepo,
+    check_user_identity,
     )
 from dulwich.tests import (
     TestCase,
@@ -927,3 +929,19 @@ class BuildRepoRootTests(TestCase):
     def test_discover_notrepo(self):
         with self.assertRaises(NotGitRepository):
             Repo.discover('/')
+
+
+class CheckUserIdentityTests(TestCase):
+
+    def test_valid(self):
+        check_user_identity(b'Me <me@example.com>')
+
+    def test_invalid(self):
+        self.assertRaises(InvalidUserIdentity,
+                          check_user_identity, b'No Email')
+        self.assertRaises(InvalidUserIdentity,
+                          check_user_identity, b'Fullname <missing')
+        self.assertRaises(InvalidUserIdentity,
+                          check_user_identity, b'Fullname missing>')
+        self.assertRaises(InvalidUserIdentity,
+                          check_user_identity, b'Fullname >order<>')