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

[PATCH v2 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 v1 [*]:
- Addressed Jordan Justen review comment

[*] https://edk2.groups.io/g/devel/message/52656

Philippe Mathieu-Daude (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 | 99 +++++++++++++++++++++++----------
 1 file changed, 69 insertions(+), 30 deletions(-)

-- 
2.21.1

[PATCH v2 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: Liming Gao <liming.gao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Signed-off-by: Philippe Mathieu-Daude <philmd@redhat.com>
---
 BaseTools/Scripts/PatchCheck.py | 82 +++++++++++++++++++++------------
 1 file changed, 53 insertions(+), 29 deletions(-)

diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
index 2a4e6f603e79..0e654345fcc8 100755
--- a/BaseTools/Scripts/PatchCheck.py
+++ b/BaseTools/Scripts/PatchCheck.py
@@ -22,6 +22,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 +173,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 v2 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: Liming Gao <liming.gao@intel.com>
Cc: Jordan Justen <jordan.l.justen@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 0e654345fcc8..a0ff5ec0038a 100755
--- a/BaseTools/Scripts/PatchCheck.py
+++ b/BaseTools/Scripts/PatchCheck.py
@@ -25,18 +25,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
@@ -173,7 +177,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 v2 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: Liming Gao <liming.gao@intel.com>
Cc: Jordan Justen <jordan.l.justen@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 a0ff5ec0038a..f0e661bfd6e3 100755
--- a/BaseTools/Scripts/PatchCheck.py
+++ b/BaseTools/Scripts/PatchCheck.py
@@ -450,6 +450,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
 
@@ -458,7 +461,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:
@@ -536,6 +539,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 v2 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: Liming Gao <liming.gao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Signed-off-by: Philippe Mathieu-Daude <philmd@redhat.com>
---
v2: Do not use "" because we use subprocess.Popen (Jordan Justen)
---
 BaseTools/Scripts/PatchCheck.py | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
index f0e661bfd6e3..c5f2f89e4d4c 100755
--- a/BaseTools/Scripts/PatchCheck.py
+++ b/BaseTools/Scripts/PatchCheck.py
@@ -560,6 +560,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:
@@ -578,6 +580,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

Re: [edk2-devel] [PATCH v2 0/4] BaseTools/PatchCheck: Check committer/author email addresses

[Bob Feng]

Please update the copyright to 2020.

For the patch set,

Reviewed-by: Bob Feng <bob.c.feng@intel.com>

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Philippe Mathieu-Daudé
Sent: Monday, January 6, 2020 6:35 PM
To: devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Laszlo Ersek <lersek@redhat.com>; Philippe Mathieu-Daude <philmd@redhat.com>
Subject: [edk2-devel] [PATCH v2 0/4] BaseTools/PatchCheck: Check committer/author email addresses

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 v1 [*]:
- Addressed Jordan Justen review comment

[*] https://edk2.groups.io/g/devel/message/52656

Philippe Mathieu-Daude (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 | 99 +++++++++++++++++++++++----------
 1 file changed, 69 insertions(+), 30 deletions(-)

--
2.21.1

Re: [edk2-devel] [PATCH v2 0/4] BaseTools/PatchCheck: Check committer/author email addresses

[Philippe Mathieu-Daudé]

On 1/9/20 10:50 AM, Feng, Bob C wrote:
> Please update the copyright to 2020.

OK will do.

> 
> For the patch set,
> 
> Reviewed-by: Bob Feng <bob.c.feng@intel.com>

Thanks!

> 
> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Philippe Mathieu-Daudé
> Sent: Monday, January 6, 2020 6:35 PM
> To: devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Laszlo Ersek <lersek@redhat.com>; Philippe Mathieu-Daude <philmd@redhat.com>
> Subject: [edk2-devel] [PATCH v2 0/4] BaseTools/PatchCheck: Check committer/author email addresses
> 
> 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 v1 [*]:
> - Addressed Jordan Justen review comment
> 
> [*] https://edk2.groups.io/g/devel/message/52656
> 
> Philippe Mathieu-Daude (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 | 99 +++++++++++++++++++++++----------
>   1 file changed, 69 insertions(+), 30 deletions(-)