Re: [Patch] BaseTools/Scripts/PatchCheck: Address false error conditions

["Laszlo Ersek" <lersek@redhat.com>]

Hello Mike,

thanks a lot for this patch!

Comments below:

On 01/10/20 23:53, Michael D Kinney wrote:
> https://bugzilla.tianocore.org/show_bug.cgi?id=2406
>
> * Always print subject line after the git commit id to make
>   it easier to know the context of warnings or errors.
> * Allow UTF-8 characters in subject line
> * Error if subject line length > 75 without CVE-xxx-xxxxx present
> * Error if subject line length > 92 with CVE-xxxx-xxxxx present
> * If body line length is > 75, then print warning instead of error.
>
> Cc: Bob Feng <bob.c.feng@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> ---
>  BaseTools/Scripts/PatchCheck.py | 57 +++++++++++++++++++++++++++++----
>  1 file changed, 51 insertions(+), 6 deletions(-)
>
> diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
> index 9668025798..ff4572bb7c 100755
> --- a/BaseTools/Scripts/PatchCheck.py
> +++ b/BaseTools/Scripts/PatchCheck.py
> @@ -1,7 +1,7 @@
>  ## @file
>  #  Check a patch for various format issues
>  #
> -#  Copyright (c) 2015 - 2019, Intel Corporation. All rights reserved.<BR>
> +#  Copyright (c) 2015 - 2020, Intel Corporation. All rights reserved.<BR>
>  #
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
>  #
> @@ -35,6 +35,8 @@ class CommitMessageCheck:
>          self.subject = subject
>          self.msg = message
>
> +        print (subject)
> +
>          self.check_contributed_under()
>          self.check_signed_off_by()
>          self.check_misc_signatures()
> @@ -179,6 +181,8 @@ class CommitMessageCheck:
>          for sig in self.sig_types:
>              self.find_signatures(sig)
>
> +    cve_re = re.compile('CVE-[0-9]{4}-[0-9]{5}[^0-9]')
> +

(1) I suggest we permit CVE IDs with 4 arbitrary digits too. That is:

  CVE-[0-9]{4}-[0-9]{4,5}[^0-9]
                    ^^^^^

Because of:

  https://cve.mitre.org/cve/identifiers/syntaxchange.html#new

For example, "CVE-2020-0001" is an existent CVE:

  https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-0001

and edk2 could get a CVE assigned for which only 4 "arbitrary digits"
were necessary. (In that case, the CNA would not pad that sequence to 5
digits.)

Now, I wouldn't like to overcomplicate this, therefore I'm not
requesting that in such cases, we also lower the inclusive limit from 92
to 91 characters, in the subject line.

(I think that this suggested update to the script/patch does not require
a reposting. Another comment below.)


>      def check_overall_format(self):
>          lines = self.msg.splitlines()
>
> @@ -196,9 +200,26 @@ class CommitMessageCheck:
>              self.error('Empty commit message!')
>              return
>
> -        if count >= 1 and len(lines[0].rstrip()) >= 72:
> -            self.error('First line of commit message (subject line) ' +
> -                       'is too long.')
> +        if count >= 1 and re.search(self.cve_re, lines[0]):
> +            #
> +            # If CVE-xxxx-xxxxx is present in subject line, then limit length of
> +            # subject line to 92 characters
> +            #
> +            if len(lines[0].rstrip()) >= 93:
> +                self.error(
> +                    'First line of commit message (subject line) is too long (%d >= 93).' %
> +                    (len(lines[0].rstrip()))
> +                    )
> +        else:
> +            #
> +            # If CVE-xxxx-xxxxx is not present in subject line, then limit
> +            # length of subject line to 75 characters
> +            #
> +            if len(lines[0].rstrip()) >= 76:
> +                self.error(
> +                    'First line of commit message (subject line) is too long (%d >= 76).' %
> +                    (len(lines[0].rstrip()))
> +                    )
>
>          if count >= 1 and len(lines[0].strip()) == 0:
>              self.error('First line of commit message (subject line) ' +
> @@ -212,7 +233,14 @@ class CommitMessageCheck:
>              if (len(lines[i]) >= 76 and
>                  len(lines[i].split()) > 1 and
>                  not lines[i].startswith('git-svn-id:')):
> -                self.error('Line %d of commit message is too long.' % (i + 1))
> +                #
> +                # Print a warning if body line is longer than 75 characters
> +                #
> +                print(
> +                    'WARNING - Line %d of commit message is too long (%d >= 76).' %
> +                    (i + 1, len(lines[i]))
> +                    )
> +                print(lines[i])
>
>          last_sig_line = None
>          for i in range(count - 1, 0, -1):
> @@ -503,8 +531,25 @@ class CheckOnePatch:
>          else:
>              self.stat = mo.group('stat')
>              self.commit_msg = mo.group('commit_message')
> +        #
> +        # Parse subject line from email header.  The subject line may be
> +        # composed of multiple parts with different encodings.  Decode and
> +        # combine all the parts to produce a single string with the contents of
> +        # the decoded subject line.
> +        #
> +        parts = email.header.decode_header(pmail.get('subject'))
> +        subject = ''
> +        for (part, encoding) in parts:
> +            if encoding:
> +                part = part.decode(encoding)
> +            else:
> +                try:
> +                    part = part.decode()
> +                except:
> +                    pass
> +            subject = subject + part
>
> -        self.commit_subject = pmail['subject'].replace('\r\n', '')
> +        self.commit_subject = subject.replace('\r\n', '')
>          self.commit_subject = self.commit_subject.replace('\n', '')
>          self.commit_subject = self.subject_prefix_re.sub('', self.commit_subject, 1)
>
>

So I've applied this patch locally, such that the most recent prior
patch for "BaseTools/Scripts/PatchCheck.py" has been commit 2649a735b249
("BaseTools/PatchCheck.py: Ignore CR and LF characters in subject
length", 2020-01-09). And I've attempted to test it against two patch
sets I have pending on the list:

[edk2-devel] [PATCH 0/2]
UefiBootManagerLib, HttpDxe: tweaks for large HTTP(S) downloads

[edk2-devel] [PATCH]
UefiCpuPkg/PiSmmCpuDxeSmm: fix 2M->4K page splitting regression for PDEs

(2) The update seems to break python2 compatibility:

> Traceback (most recent call last):
>   File "BaseTools/Scripts/PatchCheck.py", line 688, in <module>
>     sys.exit(PatchCheckApp().retval)
>   File "BaseTools/Scripts/PatchCheck.py", line 648, in __init__
>     self.process_one_arg(patch)
>   File "BaseTools/Scripts/PatchCheck.py", line 665, in process_one_arg
>     self.ok &= CheckOneArg(arg, self.count).ok
>   File "BaseTools/Scripts/PatchCheck.py", line 632, in __init__
>     checker = CheckGitCommits(param, max_count)
>   File "BaseTools/Scripts/PatchCheck.py", line 576, in __init__
>     self.ok &= CheckOnePatch(commit, patch).ok
>   File "BaseTools/Scripts/PatchCheck.py", line 451, in __init__
>     self.find_patch_pieces()
>   File "BaseTools/Scripts/PatchCheck.py", line 540, in find_patch_pieces
>     parts = email.header.decode_header(pmail.get('subject'))
> AttributeError: 'module' object has no attribute 'header'

However, the patch works very well with python3.

I've also checked a local one-off commit with a bunch of UTF-8 sequences
in the subject line. (In Hungarian we have the nonsensical phrase
"árvíztűrő tükörfúrógép ÁRVÍZTŰRŐ TÜKÖRFÚRÓGÉP" for easily testing all
the Hungarian diacritical marks, which are part of latin2. Verbatim, it
means "flood tolerant mirror drill" :) )

In order to unblock some pending patches wrt. the edk2 CI system, I'd
suggest pushing this patch at once. For that:

Tested-by: Laszlo Ersek <lersek@redhat.com>

Then, the regex improvement for (1) could be a separate, future patch.
(Normally I'd propose implementing that small change right before
pushing, but then my Tested-by would not be valid any longer.
"Reviewed-by" can survive small changes, but "Tested-by" can't.)

Similarly, python2 support (2) (*if* we care -- not saying we should
necessarily care) could be added as a subsequent patch.

In summary I'd be thankful if the patch could be pushed as-is, with my
Tested-by (and with an R-b from a BaseTools maintainer, of course).

Thank you!
Laszlo