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

[PATCH 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.

Cc: Liming Gao <liming.gao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>

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.0

[PATCH 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.0

[PATCH 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.0

[PATCH 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.0

[RFC PATCH 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>
---
RFC because I haven't checked --pretty="%cn <%ce>" works on Windows shell.

 BaseTools/Scripts/PatchCheck.py | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
index f0e661bfd6e3..3baeb3de7ba2 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.0

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

[Laszlo Ersek]

Hi,

On 01/02/20 16:25, Philippe Mathieu-Daude wrote:
> 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.
> 
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> 
> 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(-)
> 

still struggling with my email backlog, so I'm going to skip this for now.

Thanks
Laszlo

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

[Jordan Justen]

On 2020-01-02 07:25:53, Philippe Mathieu-Daude wrote:
> 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>
> ---
> RFC because I haven't checked --pretty="%cn <%ce>" works on Windows shell.
> 
>  BaseTools/Scripts/PatchCheck.py | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
> index f0e661bfd6e3..3baeb3de7ba2 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)

I think '--pretty=%cn <%ce>' ought to work without double-quotes
because the argument is separately sent via the subprocess.Popen call.
I'm not certain it will work, but it ought to. :)

-Jordan

> +
>      def run_git(self, *args):
>          cmd = [ 'git' ]
>          cmd += args
> -- 
> 2.21.0
> 

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

[Philippe Mathieu-Daudé]

On 1/3/20 11:59 PM, Jordan Justen wrote:
> On 2020-01-02 07:25:53, Philippe Mathieu-Daude wrote:
>> 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>
>> ---
>> RFC because I haven't checked --pretty="%cn <%ce>" works on Windows shell.
>>
>>   BaseTools/Scripts/PatchCheck.py | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
>> index f0e661bfd6e3..3baeb3de7ba2 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)
> 
> I think '--pretty=%cn <%ce>' ought to work without double-quotes
> because the argument is separately sent via the subprocess.Popen call.
> I'm not certain it will work, but it ought to. :)

Oh, this works like charm on Linux :)

Thanks for the tip (and review)!

Phil.

>> +
>>       def run_git(self, *args):
>>           cmd = [ 'git' ]
>>           cmd += args
>> -- 
>> 2.21.0
>>
>