* [Patch] BaseTools/Scripts/PatchCheck: Address false error conditions
@ 2020-01-10 22:53 Michael D Kinney
2020-01-13 2:13 ` Bob Feng
2020-01-13 17:31 ` Laszlo Ersek
0 siblings, 2 replies; 5+ messages in thread
From: Michael D Kinney @ 2020-01-10 22:53 UTC (permalink / raw)
To: devel; +Cc: Bob Feng, Liming Gao, Laszlo Ersek, Jordan Justen
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]')
+
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)
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Patch] BaseTools/Scripts/PatchCheck: Address false error conditions
2020-01-10 22:53 [Patch] BaseTools/Scripts/PatchCheck: Address false error conditions Michael D Kinney
@ 2020-01-13 2:13 ` Bob Feng
2020-01-13 17:31 ` Laszlo Ersek
1 sibling, 0 replies; 5+ messages in thread
From: Bob Feng @ 2020-01-13 2:13 UTC (permalink / raw)
To: Kinney, Michael D, devel@edk2.groups.io
Cc: Gao, Liming, Laszlo Ersek, Justen, Jordan L
Hi Mike,
This patch can't apply to edk2 repo, please update.
For this patch,
Reviewed-by: Bob Feng <bob.c.feng@intel.com>
Thanks,
Bob
-----Original Message-----
From: Kinney, Michael D
Sent: Saturday, January 11, 2020 6:54 AM
To: devel@edk2.groups.io
Cc: Feng, Bob C <bob.c.feng@intel.com>; Gao, Liming <liming.gao@intel.com>; Laszlo Ersek <lersek@redhat.com>; Justen, Jordan L <jordan.l.justen@intel.com>
Subject: [Patch] BaseTools/Scripts/PatchCheck: Address false error conditions
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]')
+
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)
--
2.21.0.windows.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Patch] BaseTools/Scripts/PatchCheck: Address false error conditions
2020-01-10 22:53 [Patch] BaseTools/Scripts/PatchCheck: Address false error conditions Michael D Kinney
2020-01-13 2:13 ` Bob Feng
@ 2020-01-13 17:31 ` Laszlo Ersek
2020-01-13 18:38 ` [edk2-devel] " Michael D Kinney
1 sibling, 1 reply; 5+ messages in thread
From: Laszlo Ersek @ 2020-01-13 17:31 UTC (permalink / raw)
To: Michael D Kinney, devel; +Cc: Bob Feng, Liming Gao, Jordan Justen
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [edk2-devel] [Patch] BaseTools/Scripts/PatchCheck: Address false error conditions
2020-01-13 17:31 ` Laszlo Ersek
@ 2020-01-13 18:38 ` Michael D Kinney
2020-01-14 8:48 ` Laszlo Ersek
0 siblings, 1 reply; 5+ messages in thread
From: Michael D Kinney @ 2020-01-13 18:38 UTC (permalink / raw)
To: devel@edk2.groups.io, lersek@redhat.com, Kinney, Michael D
Cc: Feng, Bob C, Gao, Liming, Justen, Jordan L
Hi Laszlo,
This patch has been pushed.
https://github.com/tianocore/edk2/pull/297
I also entered a new BZ to relax CVE pattern check.
https://bugzilla.tianocore.org/show_bug.cgi?id=2462
Mike
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On
> Behalf Of Laszlo Ersek
> Sent: Monday, January 13, 2020 9:32 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> devel@edk2.groups.io
> Cc: Feng, Bob C <bob.c.feng@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Justen, Jordan L
> <jordan.l.justen@intel.com>
> Subject: Re: [edk2-devel] [Patch]
> BaseTools/Scripts/PatchCheck: Address false error
> conditions
>
> 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
>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [edk2-devel] [Patch] BaseTools/Scripts/PatchCheck: Address false error conditions
2020-01-13 18:38 ` [edk2-devel] " Michael D Kinney
@ 2020-01-14 8:48 ` Laszlo Ersek
0 siblings, 0 replies; 5+ messages in thread
From: Laszlo Ersek @ 2020-01-14 8:48 UTC (permalink / raw)
To: Kinney, Michael D, devel@edk2.groups.io
Cc: Feng, Bob C, Gao, Liming, Justen, Jordan L
On 01/13/20 19:38, Kinney, Michael D wrote:
> Hi Laszlo,
>
> This patch has been pushed.
>
> https://github.com/tianocore/edk2/pull/297
>
> I also entered a new BZ to relax CVE pattern check.
>
> https://bugzilla.tianocore.org/show_bug.cgi?id=2462
Thank you for both!
Laszlo
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-01-14 8:48 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-01-10 22:53 [Patch] BaseTools/Scripts/PatchCheck: Address false error conditions Michael D Kinney
2020-01-13 2:13 ` Bob Feng
2020-01-13 17:31 ` Laszlo Ersek
2020-01-13 18:38 ` [edk2-devel] " Michael D Kinney
2020-01-14 8:48 ` Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox