[PATCH v4 0/4] BaseTools/PatchCheck: Check committer/author email addresses

[PATCH v4 0/4] BaseTools/PatchCheck: Check committer/author email addresses

[Philippe Mathieu-Daudé]

Last month I cleaned the git-history incorrect email
addresses: https://edk2.groups.io/g/devel/message/51834
but today I noticed more incorrect addresses got committed
(see caa917491a4..33a3293651).
To avoid having the same cleanup in the future, fix the
problem once for all by having PatchCheck doing this
automatically (on merge).

Since we already have code to check email address in
PatchCheck, factor the code out to reuse it, and
add checks for committer/author addresses.

Series only tested on Linux.

Since v3 [1]:
- Correct maintainer Cc tag in patch description

Since v2 [2]:
- Added 2020 Copyright notice in header
- Added Bob Feng Reviewed-by tag

Since v1 [3]:
- Addressed Jordan Justen review comment

[1] https://edk2.groups.io/g/devel/message/53062
[2] https://edk2.groups.io/g/devel/message/52656
[3] https://edk2.groups.io/g/devel/message/52885

Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Liming Gao <liming.gao@intel.com>

Philippe Mathieu-Daudé (4):
  BaseTools/PatchCheck.py: Extract email check code to EmailAddressCheck
  BaseTools/PatchCheck.py: Let EmailAddressCheck describe email checked
  BaseTools/PatchCheck.py: Check the patch author email address
  BaseTools/PatchCheck.py: Check the committer email address

 BaseTools/Scripts/PatchCheck.py | 100 ++++++++++++++++++++++----------
 1 file changed, 70 insertions(+), 30 deletions(-)

-- 
2.21.1

[PATCH v4 1/4] BaseTools/PatchCheck.py: Extract email check code to EmailAddressCheck

[Philippe Mathieu-Daudé]

As we are going to reuse this code out of the CommitMessageCheck
class, extract it in a new class: EmailAddressCheck.

Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Reviewed-by: Bob Feng <bob.c.feng@intel.com>
Signed-off-by: Philippe Mathieu-Daude <philmd@redhat.com>
---
 BaseTools/Scripts/PatchCheck.py | 83 +++++++++++++++++++++------------
 1 file changed, 54 insertions(+), 29 deletions(-)

diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
index 9668025798da..3b6d77081e7e 100755
--- a/BaseTools/Scripts/PatchCheck.py
+++ b/BaseTools/Scripts/PatchCheck.py
@@ -2,6 +2,7 @@
 #  Check a patch for various format issues
 #
 #  Copyright (c) 2015 - 2019, Intel Corporation. All rights reserved.<BR>
+#  Copyright (C) 2020, Red Hat, Inc.<BR>
 #
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
 #
@@ -22,6 +23,58 @@ class Verbose:
     SILENT, ONELINE, NORMAL = range(3)
     level = NORMAL
 
+class EmailAddressCheck:
+    """Checks an email address."""
+
+    def __init__(self, email):
+        self.ok = True
+
+        if email is None:
+            self.error('Email address is missing!')
+            return
+
+        self.check_email_address(email)
+
+    def error(self, *err):
+        if self.ok and Verbose.level > Verbose.ONELINE:
+            print('The email address is not valid:')
+        self.ok = False
+        if Verbose.level < Verbose.NORMAL:
+            return
+        count = 0
+        for line in err:
+            prefix = (' *', '  ')[count > 0]
+            print(prefix, line)
+            count += 1
+
+    email_re1 = re.compile(r'(?:\s*)(.*?)(\s*)<(.+)>\s*$',
+                           re.MULTILINE|re.IGNORECASE)
+
+    def check_email_address(self, email):
+        email = email.strip()
+        mo = self.email_re1.match(email)
+        if mo is None:
+            self.error("Email format is invalid: " + email.strip())
+            return
+
+        name = mo.group(1).strip()
+        if name == '':
+            self.error("Name is not provided with email address: " +
+                       email)
+        else:
+            quoted = len(name) > 2 and name[0] == '"' and name[-1] == '"'
+            if name.find(',') >= 0 and not quoted:
+                self.error('Add quotes (") around name with a comma: ' +
+                           name)
+
+        if mo.group(2) == '':
+            self.error("There should be a space between the name and " +
+                       "email address: " + email)
+
+        if mo.group(3).find(' ') >= 0:
+            self.error("The email address cannot contain a space: " +
+                       mo.group(3))
+
 class CommitMessageCheck:
     """Checks the contents of a git commit message."""
 
@@ -121,38 +174,10 @@ class CommitMessageCheck:
             if s[2] != ' ':
                 self.error("There should be a space after '" + sig + ":'")
 
-            self.check_email_address(s[3])
+            EmailAddressCheck(s[3])
 
         return sigs
 
-    email_re1 = re.compile(r'(?:\s*)(.*?)(\s*)<(.+)>\s*$',
-                           re.MULTILINE|re.IGNORECASE)
-
-    def check_email_address(self, email):
-        email = email.strip()
-        mo = self.email_re1.match(email)
-        if mo is None:
-            self.error("Email format is invalid: " + email.strip())
-            return
-
-        name = mo.group(1).strip()
-        if name == '':
-            self.error("Name is not provided with email address: " +
-                       email)
-        else:
-            quoted = len(name) > 2 and name[0] == '"' and name[-1] == '"'
-            if name.find(',') >= 0 and not quoted:
-                self.error('Add quotes (") around name with a comma: ' +
-                           name)
-
-        if mo.group(2) == '':
-            self.error("There should be a space between the name and " +
-                       "email address: " + email)
-
-        if mo.group(3).find(' ') >= 0:
-            self.error("The email address cannot contain a space: " +
-                       mo.group(3))
-
     def check_signed_off_by(self):
         sob='Signed-off-by'
         if self.msg.find(sob) < 0:
-- 
2.21.1

[PATCH v4 2/4] BaseTools/PatchCheck.py: Let EmailAddressCheck describe email checked

[Philippe Mathieu-Daudé]

We are checking different emails from the signature list. We are
going to check more. To be able to differency, add a description
field, so the error reported is clearer.

Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Reviewed-by: Bob Feng <bob.c.feng@intel.com>
Signed-off-by: Philippe Mathieu-Daude <philmd@redhat.com>
---
 BaseTools/Scripts/PatchCheck.py | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
index 3b6d77081e7e..42b923cb927e 100755
--- a/BaseTools/Scripts/PatchCheck.py
+++ b/BaseTools/Scripts/PatchCheck.py
@@ -26,18 +26,22 @@ class Verbose:
 class EmailAddressCheck:
     """Checks an email address."""
 
-    def __init__(self, email):
+    def __init__(self, email, description):
         self.ok = True
 
         if email is None:
             self.error('Email address is missing!')
             return
+        if description is None:
+            self.error('Email description is missing!')
+            return
 
+        self.description = "'" + description + "'"
         self.check_email_address(email)
 
     def error(self, *err):
         if self.ok and Verbose.level > Verbose.ONELINE:
-            print('The email address is not valid:')
+            print('The ' + self.description + ' email address is not valid:')
         self.ok = False
         if Verbose.level < Verbose.NORMAL:
             return
@@ -174,7 +178,7 @@ class CommitMessageCheck:
             if s[2] != ' ':
                 self.error("There should be a space after '" + sig + ":'")
 
-            EmailAddressCheck(s[3])
+            EmailAddressCheck(s[3], sig)
 
         return sigs
 
-- 
2.21.1

[PATCH v4 3/4] BaseTools/PatchCheck.py: Check the patch author email address

[Philippe Mathieu-Daudé]

To avoid patches committed with incorrect email address,
use the EmailAddressCheck class on the author email too.

Example:

  $ python BaseTools/Scripts/PatchCheck.py 1a04951309f
  Checking git commit: 1a04951309f
  The 'Author' email address is not valid:
  * The email address cannot contain a space: /o=Intel/ou=External \
    (FYDIBOHF25SPDLT)/cn=Recipients/cn=fe425ca7e5f4401abed22b904fe5d964

Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Reviewed-by: Bob Feng <bob.c.feng@intel.com>
Signed-off-by: Philippe Mathieu-Daude <philmd@redhat.com>
---
 BaseTools/Scripts/PatchCheck.py | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
index 42b923cb927e..e7b3c3bae47a 100755
--- a/BaseTools/Scripts/PatchCheck.py
+++ b/BaseTools/Scripts/PatchCheck.py
@@ -451,6 +451,9 @@ class CheckOnePatch:
         self.patch = patch
         self.find_patch_pieces()
 
+        email_check = EmailAddressCheck(self.author_email, 'Author')
+        email_ok = email_check.ok
+
         msg_check = CommitMessageCheck(self.commit_subject, self.commit_msg)
         msg_ok = msg_check.ok
 
@@ -459,7 +462,7 @@ class CheckOnePatch:
             diff_check = GitDiffCheck(self.diff)
             diff_ok = diff_check.ok
 
-        self.ok = msg_ok and diff_ok
+        self.ok = email_ok and msg_ok and diff_ok
 
         if Verbose.level == Verbose.ONELINE:
             if self.ok:
@@ -537,6 +540,8 @@ class CheckOnePatch:
         self.commit_subject = self.commit_subject.replace('\n', '')
         self.commit_subject = self.subject_prefix_re.sub('', self.commit_subject, 1)
 
+        self.author_email = pmail['from']
+
 class CheckGitCommits:
     """Reads patches from git based on the specified git revision range.
 
-- 
2.21.1

[PATCH v4 4/4] BaseTools/PatchCheck.py: Check the committer email address

[Philippe Mathieu-Daudé]

To avoid patches committed with incorrect email address,
use the EmailAddressCheck class on the committer email too.

Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Reviewed-by: Bob Feng <bob.c.feng@intel.com>
Signed-off-by: Philippe Mathieu-Daude <philmd@redhat.com>
---
 BaseTools/Scripts/PatchCheck.py | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
index e7b3c3bae47a..fe8e6a64f2bb 100755
--- a/BaseTools/Scripts/PatchCheck.py
+++ b/BaseTools/Scripts/PatchCheck.py
@@ -561,6 +561,8 @@ class CheckGitCommits:
                 else:
                     blank_line = True
                 print('Checking git commit:', commit)
+            email = self.read_committer_email_address_from_git(commit)
+            self.ok &= EmailAddressCheck(email, 'Committer').ok
             patch = self.read_patch_from_git(commit)
             self.ok &= CheckOnePatch(commit, patch).ok
         if not commits:
@@ -579,6 +581,10 @@ class CheckGitCommits:
         # Run git to get the commit patch
         return self.run_git('show', '--pretty=email', '--no-textconv', commit)
 
+    def read_committer_email_address_from_git(self, commit):
+        # Run git to get the committer email
+        return self.run_git('show', '--pretty=%cn <%ce>', '--no-patch', commit)
+
     def run_git(self, *args):
         cmd = [ 'git' ]
         cmd += args
-- 
2.21.1