[edk2-devel] [Patch 0/4] PatchCheck Updates

[edk2-devel] [Patch 0/4] PatchCheck Updates

[Michael D Kinney]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4679
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4680
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4693
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4694

Fix a collection of PatchCheck issues and add a new PatchCheck
featue to check for a commit that modifies files in multiple
packages with option to disable the multiple package check as
a command line option or a commit message tag.

* Reject patches that match Author email "devel@edk2.groups.io"
* Update the current check for " via Groups.Io" to perform a
  case insensitive match. It appears that groups.io has changed the
  format of this string to use all lower case.
* Update logic in CommitMessageCheck class to return errors
  detected in commit message signatures instead of silently 
  passing the check.
* Genenerate an error if no Cc tags are present to make sure
  all patches Cc the required maintainers/reviewers.

Cc: Rebecca Cran <rebecca@bsdio.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Yuwei Chen <yuwei.chen@intel.com>
Cc: Michael Kubacki <mikuback@linux.microsoft.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>

Michael D Kinney (4):
  BaseTools/Scripts/PatchCheck: Update Author checks
  BaseTools/Scripts/PatchCheck: Return CommitMessageCheck errors
  BaseTools/Scripts/PatchCheck: Error if no Cc tags are present
  BaseTools/Scripts/PatchCheck: Error if commit modifies multiple
    packages

 BaseTools/Scripts/PatchCheck.py | 91 +++++++++++++++++++++++++++++++--
 1 file changed, 86 insertions(+), 5 deletions(-)

-- 
2.40.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115568): https://edk2.groups.io/g/devel/message/115568
Mute This Topic: https://groups.io/mt/104434581/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [Patch 1/4] BaseTools/Scripts/PatchCheck: Update Author checks

[Michael D Kinney]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4680

* Reject patches that match Author email "devel@edk2.groups.io"
* Update the current check for " via Groups.Io" to perform a
  case insensitive match. It appears that groups.io has changed the
  format of this string to use all lower case.

Cc: Rebecca Cran <rebecca@bsdio.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Yuwei Chen <yuwei.chen@intel.com>
Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
Reviewed-by: Rebecca Cran <rebecca@bsdio.com>
Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
---
 BaseTools/Scripts/PatchCheck.py | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
index 1675dcbd7321..e600e0be440f 100755
--- a/BaseTools/Scripts/PatchCheck.py
+++ b/BaseTools/Scripts/PatchCheck.py
@@ -85,7 +85,11 @@ class EmailAddressCheck:
             self.error("The email address cannot contain a space: " +
                        mo.group(3))
 
-        if ' via Groups.Io' in name and mo.group(3).endswith('@groups.io'):
+        if mo.group(3) == 'devel@edk2.groups.io':
+            self.error("Email rewritten by lists DMARC / DKIM / SPF: " +
+                       email)
+
+        if ' via groups.io' in name.lower() and mo.group(3).endswith('@groups.io'):
             self.error("Email rewritten by lists DMARC / DKIM / SPF: " +
                        email)
 
-- 
2.40.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115569): https://edk2.groups.io/g/devel/message/115569
Mute This Topic: https://groups.io/mt/104434582/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Patch 1/4] BaseTools/Scripts/PatchCheck: Update Author checks

[Rebecca Cran]

For the series:

Reviewed-by: Rebecca Cran <rebecca@bsdio.com>

-- 
Rebecca Cran


On 2/18/2024 1:59 PM, Michael D Kinney wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4680
>
> * Reject patches that match Author email "devel@edk2.groups.io"
> * Update the current check for " via Groups.Io" to perform a
>    case insensitive match. It appears that groups.io has changed the
>    format of this string to use all lower case.
>
> Cc: Rebecca Cran <rebecca@bsdio.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Bob Feng <bob.c.feng@intel.com>
> Cc: Yuwei Chen <yuwei.chen@intel.com>
> Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> Reviewed-by: Rebecca Cran <rebecca@bsdio.com>
> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>   BaseTools/Scripts/PatchCheck.py | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
> index 1675dcbd7321..e600e0be440f 100755
> --- a/BaseTools/Scripts/PatchCheck.py
> +++ b/BaseTools/Scripts/PatchCheck.py
> @@ -85,7 +85,11 @@ class EmailAddressCheck:
>               self.error("The email address cannot contain a space: " +
>                          mo.group(3))
>   
> -        if ' via Groups.Io' in name and mo.group(3).endswith('@groups.io'):
> +        if mo.group(3) == 'devel@edk2.groups.io':
> +            self.error("Email rewritten by lists DMARC / DKIM / SPF: " +
> +                       email)
> +
> +        if ' via groups.io' in name.lower() and mo.group(3).endswith('@groups.io'):
>               self.error("Email rewritten by lists DMARC / DKIM / SPF: " +
>                          email)
>   




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116057): https://edk2.groups.io/g/devel/message/116057
Mute This Topic: https://groups.io/mt/104434582/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [Patch 2/4] BaseTools/Scripts/PatchCheck: Return CommitMessageCheck errors

[Michael D Kinney]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4693

Commit signatures are checked and error messages are logged but
errors are not captured and returned from find_signatures() in the
CommitMessageCheck class. This causes signature errors to be
silently ignored by CI.

Update logic in CommitMessageCheck class to return errors
detected in commit message signatures.

Cc: Rebecca Cran <rebecca@bsdio.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Yuwei Chen <yuwei.chen@intel.com>
Cc: Michael Kubacki <mikuback@linux.microsoft.com>
Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
---
 BaseTools/Scripts/PatchCheck.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
index e600e0be440f..158a2b30a5ce 100755
--- a/BaseTools/Scripts/PatchCheck.py
+++ b/BaseTools/Scripts/PatchCheck.py
@@ -202,7 +202,7 @@ class CommitMessageCheck:
             if s[2] != ' ':
                 self.error("There should be a space after '" + sig + ":'")
 
-            EmailAddressCheck(s[3], sig)
+            self.ok &= EmailAddressCheck(s[3], sig).ok
 
         return sigs
 
-- 
2.40.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115570): https://edk2.groups.io/g/devel/message/115570
Mute This Topic: https://groups.io/mt/104434583/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Patch 2/4] BaseTools/Scripts/PatchCheck: Return CommitMessageCheck errors

[Michael Kubacki]

Reviewed-by: Michael Kubacki <michael.kubacki@microsoft.com>

On 2/18/2024 3:59 PM, Michael D Kinney wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4693
> 
> Commit signatures are checked and error messages are logged but
> errors are not captured and returned from find_signatures() in the
> CommitMessageCheck class. This causes signature errors to be
> silently ignored by CI.
> 
> Update logic in CommitMessageCheck class to return errors
> detected in commit message signatures.
> 
> Cc: Rebecca Cran <rebecca@bsdio.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Bob Feng <bob.c.feng@intel.com>
> Cc: Yuwei Chen <yuwei.chen@intel.com>
> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> ---
>   BaseTools/Scripts/PatchCheck.py | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
> index e600e0be440f..158a2b30a5ce 100755
> --- a/BaseTools/Scripts/PatchCheck.py
> +++ b/BaseTools/Scripts/PatchCheck.py
> @@ -202,7 +202,7 @@ class CommitMessageCheck:
>               if s[2] != ' ':
>                   self.error("There should be a space after '" + sig + ":'")
>   
> -            EmailAddressCheck(s[3], sig)
> +            self.ok &= EmailAddressCheck(s[3], sig).ok
>   
>           return sigs
>   


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115901): https://edk2.groups.io/g/devel/message/115901
Mute This Topic: https://groups.io/mt/104434583/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [Patch 3/4] BaseTools/Scripts/PatchCheck: Error if no Cc tags are present

[Michael D Kinney]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4694

If no Cc tags are detected in a commit message, then generate an
error. All patches sent for review are required to provide the set
of maintainers and reviewers responsible for the directories/files
modified. The set of maintainers and reviewers are documented in
Maintainers.txt and can be retrieved using the script
BaseTools/Scripts/GetMaintainer.py.

Cc: Rebecca Cran <rebecca@bsdio.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Yuwei Chen <yuwei.chen@intel.com>
Cc: Michael Kubacki <mikuback@linux.microsoft.com>
Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
---
 BaseTools/Scripts/PatchCheck.py | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
index 158a2b30a5ce..415198e3824e 100755
--- a/BaseTools/Scripts/PatchCheck.py
+++ b/BaseTools/Scripts/PatchCheck.py
@@ -229,8 +229,10 @@ class CommitMessageCheck:
         )
 
     def check_misc_signatures(self):
-        for sig in self.sig_types:
-            self.find_signatures(sig)
+        for sigtype in self.sig_types:
+            sigs = self.find_signatures(sigtype)
+            if sigtype == 'Cc' and len(sigs) == 0:
+                self.error('No Cc: tags for maintainers/reviewers found!')
 
     cve_re = re.compile('CVE-[0-9]{4}-[0-9]{5}[^0-9]')
 
-- 
2.40.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115571): https://edk2.groups.io/g/devel/message/115571
Mute This Topic: https://groups.io/mt/104434584/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Patch 3/4] BaseTools/Scripts/PatchCheck: Error if no Cc tags are present

[Ard Biesheuvel]

Hello Mike,

I understand the desire to be pedantic about cc'ing the right
maintainers, but I'm not convinced this is the way.

- the presence of a cc: tag does not guarantee that the person was
cc'ed - only git send-email will take CC:s in the message body into
account by default (but this can also be disabled), but generally, the
sender has to ensure the cc list is copied into the cc: field
- the absence of a cc: tag does not imply that the person was not cc'ed,

- in Linux, the cc: tag has slightly different semantics from the ones
we appear to be assuming here: a cc tag in patch going into the
repository is a statement by the maintainer that the person in
question has been cc'ed, may have some 'jurisdiction' over the area,
but hasn't bothered to respond. IOW, it is to record the fact that
this person has been given the opportunity to respond.

Then there is the matter of a maintainer that has reviewed the patch
themselves. I usually remove the cc lines listing people that have
reviewed/acked/tested the patch, as those tags already convey that the
person is aware of it cc'ed or not.

So perhaps it would be better to make this check part of the
contributor workflow but not the GitHub PR/CI workflow?


On Sun, 18 Feb 2024 at 22:00, Michael D Kinney
<michael.d.kinney@intel.com> wrote:
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4694
>
> If no Cc tags are detected in a commit message, then generate an
> error. All patches sent for review are required to provide the set
> of maintainers and reviewers responsible for the directories/files
> modified. The set of maintainers and reviewers are documented in
> Maintainers.txt and can be retrieved using the script
> BaseTools/Scripts/GetMaintainer.py.
>
> Cc: Rebecca Cran <rebecca@bsdio.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Bob Feng <bob.c.feng@intel.com>
> Cc: Yuwei Chen <yuwei.chen@intel.com>
> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> ---
>  BaseTools/Scripts/PatchCheck.py | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
> index 158a2b30a5ce..415198e3824e 100755
> --- a/BaseTools/Scripts/PatchCheck.py
> +++ b/BaseTools/Scripts/PatchCheck.py
> @@ -229,8 +229,10 @@ class CommitMessageCheck:
>          )
>
>      def check_misc_signatures(self):
> -        for sig in self.sig_types:
> -            self.find_signatures(sig)
> +        for sigtype in self.sig_types:
> +            sigs = self.find_signatures(sigtype)
> +            if sigtype == 'Cc' and len(sigs) == 0:
> +                self.error('No Cc: tags for maintainers/reviewers found!')
>
>      cve_re = re.compile('CVE-[0-9]{4}-[0-9]{5}[^0-9]')
>
> --
> 2.40.1.windows.1
>
>
>
> 
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115653): https://edk2.groups.io/g/devel/message/115653
Mute This Topic: https://groups.io/mt/104434584/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Patch 3/4] BaseTools/Scripts/PatchCheck: Error if no Cc tags are present

[Michael D Kinney]

Hi Ard,

I suspected this one BZ/patch would get some discussion.

This is an attempt to address the fundamental issue that we do not
get timely reviews of patches.  When I look at the ones that are
delayed, in many cases, there are no Cc lines in the commit message
and no Cc in the email.

There are many times I have seen extra emails reminding the author
to Cc the maintainers/reviewers.

I agree that doing this in CI is not best location.

If a submitter does open a PR to use EDK II CI to test their
changes before sending email reviews, then it would help. If the
changes are sent for email review first, then it would not help.

Best regards,

Mike

> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Tuesday, February 20, 2024 6:49 AM
> To: devel@edk2.groups.io; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Cc: Rebecca Cran <rebecca@bsdio.com>; Liming Gao
> <gaoliming@byosoft.com.cn>; Feng, Bob C <bob.c.feng@intel.com>; Chen,
> Christine <yuwei.chen@intel.com>; Michael Kubacki
> <mikuback@linux.microsoft.com>
> Subject: Re: [edk2-devel] [Patch 3/4] BaseTools/Scripts/PatchCheck:
> Error if no Cc tags are present
> 
> Hello Mike,
> 
> I understand the desire to be pedantic about cc'ing the right
> maintainers, but I'm not convinced this is the way.
> 
> - the presence of a cc: tag does not guarantee that the person was
> cc'ed - only git send-email will take CC:s in the message body into
> account by default (but this can also be disabled), but generally, the
> sender has to ensure the cc list is copied into the cc: field
> - the absence of a cc: tag does not imply that the person was not
> cc'ed,
> 
> - in Linux, the cc: tag has slightly different semantics from the ones
> we appear to be assuming here: a cc tag in patch going into the
> repository is a statement by the maintainer that the person in
> question has been cc'ed, may have some 'jurisdiction' over the area,
> but hasn't bothered to respond. IOW, it is to record the fact that
> this person has been given the opportunity to respond.
> 
> Then there is the matter of a maintainer that has reviewed the patch
> themselves. I usually remove the cc lines listing people that have
> reviewed/acked/tested the patch, as those tags already convey that the
> person is aware of it cc'ed or not.
> 
> So perhaps it would be better to make this check part of the
> contributor workflow but not the GitHub PR/CI workflow?
> 
> 
> On Sun, 18 Feb 2024 at 22:00, Michael D Kinney
> <michael.d.kinney@intel.com> wrote:
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4694
> >
> > If no Cc tags are detected in a commit message, then generate an
> > error. All patches sent for review are required to provide the set
> > of maintainers and reviewers responsible for the directories/files
> > modified. The set of maintainers and reviewers are documented in
> > Maintainers.txt and can be retrieved using the script
> > BaseTools/Scripts/GetMaintainer.py.
> >
> > Cc: Rebecca Cran <rebecca@bsdio.com>
> > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > Cc: Bob Feng <bob.c.feng@intel.com>
> > Cc: Yuwei Chen <yuwei.chen@intel.com>
> > Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> > Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> > ---
> >  BaseTools/Scripts/PatchCheck.py | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/BaseTools/Scripts/PatchCheck.py
> b/BaseTools/Scripts/PatchCheck.py
> > index 158a2b30a5ce..415198e3824e 100755
> > --- a/BaseTools/Scripts/PatchCheck.py
> > +++ b/BaseTools/Scripts/PatchCheck.py
> > @@ -229,8 +229,10 @@ class CommitMessageCheck:
> >          )
> >
> >      def check_misc_signatures(self):
> > -        for sig in self.sig_types:
> > -            self.find_signatures(sig)
> > +        for sigtype in self.sig_types:
> > +            sigs = self.find_signatures(sigtype)
> > +            if sigtype == 'Cc' and len(sigs) == 0:
> > +                self.error('No Cc: tags for maintainers/reviewers
> found!')
> >
> >      cve_re = re.compile('CVE-[0-9]{4}-[0-9]{5}[^0-9]')
> >
> > --
> > 2.40.1.windows.1
> >
> >
> >
> > 
> >
> >


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115680): https://edk2.groups.io/g/devel/message/115680
Mute This Topic: https://groups.io/mt/104434584/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Patch 3/4] BaseTools/Scripts/PatchCheck: Error if no Cc tags are present

[Laszlo Ersek]

On 2/20/24 15:48, Ard Biesheuvel wrote:
> Hello Mike,
> 
> I understand the desire to be pedantic about cc'ing the right
> maintainers, but I'm not convinced this is the way.
> 
> - the presence of a cc: tag does not guarantee that the person was
> cc'ed - only git send-email will take CC:s in the message body into
> account by default (but this can also be disabled), but generally, the
> sender has to ensure the cc list is copied into the cc: field
> - the absence of a cc: tag does not imply that the person was not cc'ed,
> 
> - in Linux, the cc: tag has slightly different semantics from the ones
> we appear to be assuming here: a cc tag in patch going into the
> repository is a statement by the maintainer that the person in
> question has been cc'ed, may have some 'jurisdiction' over the area,
> but hasn't bothered to respond. IOW, it is to record the fact that
> this person has been given the opportunity to respond.
> 
> Then there is the matter of a maintainer that has reviewed the patch
> themselves. I usually remove the cc lines listing people that have
> reviewed/acked/tested the patch, as those tags already convey that the
> person is aware of it cc'ed or not.

I've noticed this (on patches you merged), and -- not having similar
maintainer experience in Linux -- I was surprised. I more or less
deduced the intent, but it felt a bit foreign (or at least novel!) to edk2.

To me, the greatest benefits of Cc's in commit messages are (as opposed
to command line specified Cc's):

- fine-grained: each patch can target a specific set of reviewers /
maintainers,

- long-lived: the CC list survives rebases / v2, v3 etc iterations! (Of
course, if a patch undergoes serious scope changes when revised, then
the Cc list will have to be updated manually. But that's quite rare.)

> 
> So perhaps it would be better to make this check part of the
> contributor workflow but not the GitHub PR/CI workflow?

I agree that adding Cc's to the commit message body is not fool-proof
(like you explain), but like Mike, I have no better idea for preventing
contributors from posting patches without properly CC'ing
reviewers/maintainers (be it with whatever particular CC'ing method they
prefer).

I tend to run PatchCheck locally (not solely relying on CI for that --
PatchCheck is quick and has no intrusive dependencies, plus seeing CI
fail just because of PatchCheck is super irritating), so in my workflow,
this patch would fit well. Of course, with the same effort of
remembering to run PatchCheck locally, I also remember to add Cc's in
the first place...

I admit that reviewer assignment is a significant shortcoming of the
email-based workflow. What we'd really need is a groups.io-level action
:) -- if the subject contains PATCH or Patch in brackets, but the body
lacks ^Cc or ^CC, *reject* the email. (Rejection gives the sender an
explanation.) Alas, rejection is currently only a manual action that's
available to moderators (and only on messages for senders that have not
been unmoderated yet).

So, my take: not perfect, but much better than nothing.

Laszlo

> 
> 
> On Sun, 18 Feb 2024 at 22:00, Michael D Kinney
> <michael.d.kinney@intel.com> wrote:
>>
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4694
>>
>> If no Cc tags are detected in a commit message, then generate an
>> error. All patches sent for review are required to provide the set
>> of maintainers and reviewers responsible for the directories/files
>> modified. The set of maintainers and reviewers are documented in
>> Maintainers.txt and can be retrieved using the script
>> BaseTools/Scripts/GetMaintainer.py.
>>
>> Cc: Rebecca Cran <rebecca@bsdio.com>
>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>> Cc: Bob Feng <bob.c.feng@intel.com>
>> Cc: Yuwei Chen <yuwei.chen@intel.com>
>> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
>> Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
>> ---
>>  BaseTools/Scripts/PatchCheck.py | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
>> index 158a2b30a5ce..415198e3824e 100755
>> --- a/BaseTools/Scripts/PatchCheck.py
>> +++ b/BaseTools/Scripts/PatchCheck.py
>> @@ -229,8 +229,10 @@ class CommitMessageCheck:
>>          )
>>
>>      def check_misc_signatures(self):
>> -        for sig in self.sig_types:
>> -            self.find_signatures(sig)
>> +        for sigtype in self.sig_types:
>> +            sigs = self.find_signatures(sigtype)
>> +            if sigtype == 'Cc' and len(sigs) == 0:
>> +                self.error('No Cc: tags for maintainers/reviewers found!')
>>
>>      cve_re = re.compile('CVE-[0-9]{4}-[0-9]{5}[^0-9]')
>>
>> --
>> 2.40.1.windows.1
>>
>>
>>
>>
>>
>>
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115745): https://edk2.groups.io/g/devel/message/115745
Mute This Topic: https://groups.io/mt/104434584/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Patch 3/4] BaseTools/Scripts/PatchCheck: Error if no Cc tags are present

[Ard Biesheuvel]

On Wed, 21 Feb 2024 at 23:16, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 2/20/24 15:48, Ard Biesheuvel wrote:
> > Hello Mike,
> >
> > I understand the desire to be pedantic about cc'ing the right
> > maintainers, but I'm not convinced this is the way.
> >
> > - the presence of a cc: tag does not guarantee that the person was
> > cc'ed - only git send-email will take CC:s in the message body into
> > account by default (but this can also be disabled), but generally, the
> > sender has to ensure the cc list is copied into the cc: field
> > - the absence of a cc: tag does not imply that the person was not cc'ed,
> >
> > - in Linux, the cc: tag has slightly different semantics from the ones
> > we appear to be assuming here: a cc tag in patch going into the
> > repository is a statement by the maintainer that the person in
> > question has been cc'ed, may have some 'jurisdiction' over the area,
> > but hasn't bothered to respond. IOW, it is to record the fact that
> > this person has been given the opportunity to respond.
> >
> > Then there is the matter of a maintainer that has reviewed the patch
> > themselves. I usually remove the cc lines listing people that have
> > reviewed/acked/tested the patch, as those tags already convey that the
> > person is aware of it cc'ed or not.
>
> I've noticed this (on patches you merged), and -- not having similar
> maintainer experience in Linux -- I was surprised. I more or less
> deduced the intent, but it felt a bit foreign (or at least novel!) to edk2.
>
> To me, the greatest benefits of Cc's in commit messages are (as opposed
> to command line specified Cc's):
>
> - fine-grained: each patch can target a specific set of reviewers /
> maintainers,
>
> - long-lived: the CC list survives rebases / v2, v3 etc iterations! (Of
> course, if a patch undergoes serious scope changes when revised, then
> the Cc list will have to be updated manually. But that's quite rare.)
>
> >
> > So perhaps it would be better to make this check part of the
> > contributor workflow but not the GitHub PR/CI workflow?
>
> I agree that adding Cc's to the commit message body is not fool-proof
> (like you explain), but like Mike, I have no better idea for preventing
> contributors from posting patches without properly CC'ing
> reviewers/maintainers (be it with whatever particular CC'ing method they
> prefer).
>
> I tend to run PatchCheck locally (not solely relying on CI for that --
> PatchCheck is quick and has no intrusive dependencies, plus seeing CI
> fail just because of PatchCheck is super irritating), so in my workflow,
> this patch would fit well. Of course, with the same effort of
> remembering to run PatchCheck locally, I also remember to add Cc's in
> the first place...
>
> I admit that reviewer assignment is a significant shortcoming of the
> email-based workflow. What we'd really need is a groups.io-level action
> :) -- if the subject contains PATCH or Patch in brackets, but the body
> lacks ^Cc or ^CC, *reject* the email. (Rejection gives the sender an
> explanation.) Alas, rejection is currently only a manual action that's
> available to moderators (and only on messages for senders that have not
> been unmoderated yet).
>
> So, my take: not perfect, but much better than nothing.
>

Yeah, I can't argue with that. I agree that it is very annoying that
patches don't get cc'ed to the right people. (It is even more annoying
that many maintainers [including myself at times - mea culpa] don't
bother to respond, but that is a different matter)

So let's try this, and in case it does more harm than good, we can
always back it out again (or make modifications to the logic)


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115752): https://edk2.groups.io/g/devel/message/115752
Mute This Topic: https://groups.io/mt/104434584/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Patch 3/4] BaseTools/Scripts/PatchCheck: Error if no Cc tags are present

[Michael Kubacki]

Reviewed-by: Michael Kubacki <michael.kubacki@microsoft.com>

On 2/18/2024 3:59 PM, Michael D Kinney wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4694
> 
> If no Cc tags are detected in a commit message, then generate an
> error. All patches sent for review are required to provide the set
> of maintainers and reviewers responsible for the directories/files
> modified. The set of maintainers and reviewers are documented in
> Maintainers.txt and can be retrieved using the script
> BaseTools/Scripts/GetMaintainer.py.
> 
> Cc: Rebecca Cran <rebecca@bsdio.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Bob Feng <bob.c.feng@intel.com>
> Cc: Yuwei Chen <yuwei.chen@intel.com>
> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> ---
>   BaseTools/Scripts/PatchCheck.py | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
> index 158a2b30a5ce..415198e3824e 100755
> --- a/BaseTools/Scripts/PatchCheck.py
> +++ b/BaseTools/Scripts/PatchCheck.py
> @@ -229,8 +229,10 @@ class CommitMessageCheck:
>           )
>   
>       def check_misc_signatures(self):
> -        for sig in self.sig_types:
> -            self.find_signatures(sig)
> +        for sigtype in self.sig_types:
> +            sigs = self.find_signatures(sigtype)
> +            if sigtype == 'Cc' and len(sigs) == 0:
> +                self.error('No Cc: tags for maintainers/reviewers found!')
>   
>       cve_re = re.compile('CVE-[0-9]{4}-[0-9]{5}[^0-9]')
>   


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115902): https://edk2.groups.io/g/devel/message/115902
Mute This Topic: https://groups.io/mt/104434584/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [Patch 4/4] BaseTools/Scripts/PatchCheck: Error if commit modifies multiple packages

[Michael D Kinney]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4679

Update PatchCheck.py to evaluate all the files modified in each commit
and generate an error if:
* A commit adds/modifies files in multiple package directories
* A commit adds/modifies files in multiple non-package directories
* A commit adds/modifies files in both a package and a non-package
  directory
* A commit deletes files from multiple package directories
* A commit deletes files from multiple non-package directories
* A commit deletes files from both a package and a non-package
  directory

Modifications to files in the root of the repository are not
evaluated.

This check is skipped if PatchCheck.py is run on a patch file or
input from stdin because this multiple package commit check depends
on information from a git repository.

If --ignore-multi-package option is set, then reduce the multiple
package commit check from an error to a warning for all commits in
the commit range provided to PatchCheck.py.

Add check for a 'Continuous-integration-options:' commit message
tag that allows one or more options to be specified at the individual
commit scope to enable/disable continuous integration checks. This
tag must start at the beginning of a commit message line and may
appear more than once in a commit message.

Add support for a Continuous-integration-options tag value of
'PatchCheck.ignore-multi-package' that reduces the multiple package
commit check from an error to a warning for the specific commits that
specify this option.  Example:

  Continuous-integration-options: PatchCheck.ignore-multi-package

The set of packages are found by searching for DEC files in a git
repository. The list of DEC files in a git repository is collected
with the following git command:

  git ls-files *.dec

The set of files added/modified by each commit is found using the
following git command:

  git diff-tree --no-commit-id --name-only --diff-filter=AM -r <commit>

The set of files deleted by each commit is found using the
following git command:

  git diff-tree --no-commit-id --name-only --diff-filter=D -r <commit>

Cc: Rebecca Cran <rebecca@bsdio.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Yuwei Chen <yuwei.chen@intel.com>
Cc: Michael Kubacki <mikuback@linux.microsoft.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
---
 BaseTools/Scripts/PatchCheck.py | 77 ++++++++++++++++++++++++++++++++-
 1 file changed, 76 insertions(+), 1 deletion(-)

diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
index 415198e3824e..a3b812fb7324 100755
--- a/BaseTools/Scripts/PatchCheck.py
+++ b/BaseTools/Scripts/PatchCheck.py
@@ -28,6 +28,7 @@ class Verbose:
 
 class PatchCheckConf:
     ignore_change_id = False
+    ignore_multi_package = False
 
 class EmailAddressCheck:
     """Checks an email address."""
@@ -98,6 +99,7 @@ class CommitMessageCheck:
 
     def __init__(self, subject, message, author_email):
         self.ok = True
+        self.ignore_multi_package = False
 
         if subject is None and  message is None:
             self.error('Commit message is missing!')
@@ -120,6 +122,7 @@ class CommitMessageCheck:
             self.check_overall_format()
             if not PatchCheckConf.ignore_change_id:
                 self.check_change_id_format()
+            self.check_ci_options_format()
         self.report_message_result()
 
     url = 'https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format'
@@ -324,6 +327,15 @@ class CommitMessageCheck:
             self.error('\"%s\" found in commit message:' % cid)
             return
 
+    def check_ci_options_format(self):
+        cio='Continuous-integration-options:'
+        for line in self.msg.splitlines():
+            if not line.startswith(cio):
+                continue
+            options = line.split(':', 1)[1].split()
+            if 'PatchCheck.ignore-multi-package' in options:
+                self.ignore_multi_package = True
+
 (START, PRE_PATCH, PATCH) = range(3)
 
 class GitDiffCheck:
@@ -561,6 +573,7 @@ class CheckOnePatch:
 
         msg_check = CommitMessageCheck(self.commit_subject, self.commit_msg, self.author_email)
         msg_ok = msg_check.ok
+        self.ignore_multi_package = msg_check.ignore_multi_package
 
         diff_ok = True
         if self.diff is not None:
@@ -671,6 +684,7 @@ class CheckGitCommits:
     """
 
     def __init__(self, rev_spec, max_count):
+        dec_files = self.read_dec_files_from_git()
         commits = self.read_commit_list_from_git(rev_spec, max_count)
         if len(commits) == 1 and Verbose.level > Verbose.ONELINE:
             commits = [ rev_spec ]
@@ -686,10 +700,66 @@ class CheckGitCommits:
             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
+            check_patch = CheckOnePatch(commit, patch)
+            self.ok &= check_patch.ok
+            ignore_multi_package = check_patch.ignore_multi_package
+            if PatchCheckConf.ignore_multi_package:
+                ignore_multi_package = True
+            prefix = 'WARNING: ' if ignore_multi_package else ''
+            check_parent = self.check_parent_packages (dec_files, commit, prefix)
+            if not ignore_multi_package:
+                self.ok &= check_parent
+
         if not commits:
             print("Couldn't find commit matching: '{}'".format(rev_spec))
 
+    def check_parent_packages(self, dec_files, commit, prefix):
+        ok = True
+        modified = self.get_parent_packages (dec_files, commit, 'AM')
+        if len (modified) > 1:
+            print("{}The commit adds/modifies files in multiple packages:".format(prefix))
+            print(" *", '\n * '.join(modified))
+            ok = False
+        deleted = self.get_parent_packages (dec_files, commit, 'D')
+        if len (deleted) > 1:
+            print("{}The commit deletes files from multiple packages:".format(prefix))
+            print(" *", '\n * '.join(deleted))
+            ok = False
+        return ok
+
+    def get_parent_packages(self, dec_files, commit, filter):
+        filelist = self.read_files_modified_from_git (commit, filter)
+        parents = set()
+        for file in filelist:
+            dec_found = False
+            for dec_file in dec_files:
+                if os.path.commonpath([dec_file, file]):
+                    dec_found = True
+                    parents.add(dec_file)
+            if not dec_found and os.path.dirname (file):
+                # No DEC file found and file is in a subdir
+                # Covers BaseTools, .github, .azurepipelines, .pytool
+                parents.add(file.split('/')[0])
+        return list(parents)
+
+    def read_dec_files_from_git(self):
+        # run git ls-files *.dec
+        out = self.run_git('ls-files', '*.dec')
+        # return list of .dec files
+        try:
+            return out.split()
+        except:
+            return []
+
+    def read_files_modified_from_git(self, commit, filter):
+        # run git diff-tree --no-commit-id --name-only -r <commit>
+        out = self.run_git('diff-tree', '--no-commit-id', '--name-only',
+                           '--diff-filter=' + filter, '-r', commit)
+        try:
+            return out.split()
+        except:
+            return []
+
     def read_commit_list_from_git(self, rev_spec, max_count):
         # Run git to get the commit patch
         cmd = [ 'rev-list', '--abbrev-commit', '--no-walk' ]
@@ -800,6 +870,9 @@ class PatchCheckApp:
         group.add_argument("--ignore-change-id",
                            action="store_true",
                            help="Ignore the presence of 'Change-Id:' tags in commit message")
+        group.add_argument("--ignore-multi-package",
+                           action="store_true",
+                           help="Ignore if commit modifies files in multiple packages")
         self.args = parser.parse_args()
         if self.args.oneline:
             Verbose.level = Verbose.ONELINE
@@ -807,6 +880,8 @@ class PatchCheckApp:
             Verbose.level = Verbose.SILENT
         if self.args.ignore_change_id:
             PatchCheckConf.ignore_change_id = True
+        if self.args.ignore_multi_package:
+            PatchCheckConf.ignore_multi_package = True
 
 if __name__ == "__main__":
     sys.exit(PatchCheckApp().retval)
-- 
2.40.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115572): https://edk2.groups.io/g/devel/message/115572
Mute This Topic: https://groups.io/mt/104434585/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Patch 4/4] BaseTools/Scripts/PatchCheck: Error if commit modifies multiple packages

[Michael Kubacki]

Reviewed-by: Michael Kubacki <michael.kubacki@microsoft.com>

On 2/18/2024 3:59 PM, Michael D Kinney wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4679
> 
> Update PatchCheck.py to evaluate all the files modified in each commit
> and generate an error if:
> * A commit adds/modifies files in multiple package directories
> * A commit adds/modifies files in multiple non-package directories
> * A commit adds/modifies files in both a package and a non-package
>    directory
> * A commit deletes files from multiple package directories
> * A commit deletes files from multiple non-package directories
> * A commit deletes files from both a package and a non-package
>    directory
> 
> Modifications to files in the root of the repository are not
> evaluated.
> 
> This check is skipped if PatchCheck.py is run on a patch file or
> input from stdin because this multiple package commit check depends
> on information from a git repository.
> 
> If --ignore-multi-package option is set, then reduce the multiple
> package commit check from an error to a warning for all commits in
> the commit range provided to PatchCheck.py.
> 
> Add check for a 'Continuous-integration-options:' commit message
> tag that allows one or more options to be specified at the individual
> commit scope to enable/disable continuous integration checks. This
> tag must start at the beginning of a commit message line and may
> appear more than once in a commit message.
> 
> Add support for a Continuous-integration-options tag value of
> 'PatchCheck.ignore-multi-package' that reduces the multiple package
> commit check from an error to a warning for the specific commits that
> specify this option.  Example:
> 
>    Continuous-integration-options: PatchCheck.ignore-multi-package
> 
> The set of packages are found by searching for DEC files in a git
> repository. The list of DEC files in a git repository is collected
> with the following git command:
> 
>    git ls-files *.dec
> 
> The set of files added/modified by each commit is found using the
> following git command:
> 
>    git diff-tree --no-commit-id --name-only --diff-filter=AM -r <commit>
> 
> The set of files deleted by each commit is found using the
> following git command:
> 
>    git diff-tree --no-commit-id --name-only --diff-filter=D -r <commit>
> 
> Cc: Rebecca Cran <rebecca@bsdio.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Bob Feng <bob.c.feng@intel.com>
> Cc: Yuwei Chen <yuwei.chen@intel.com>
> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> ---
>   BaseTools/Scripts/PatchCheck.py | 77 ++++++++++++++++++++++++++++++++-
>   1 file changed, 76 insertions(+), 1 deletion(-)
> 
> diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
> index 415198e3824e..a3b812fb7324 100755
> --- a/BaseTools/Scripts/PatchCheck.py
> +++ b/BaseTools/Scripts/PatchCheck.py
> @@ -28,6 +28,7 @@ class Verbose:
>   
>   class PatchCheckConf:
>       ignore_change_id = False
> +    ignore_multi_package = False
>   
>   class EmailAddressCheck:
>       """Checks an email address."""
> @@ -98,6 +99,7 @@ class CommitMessageCheck:
>   
>       def __init__(self, subject, message, author_email):
>           self.ok = True
> +        self.ignore_multi_package = False
>   
>           if subject is None and  message is None:
>               self.error('Commit message is missing!')
> @@ -120,6 +122,7 @@ class CommitMessageCheck:
>               self.check_overall_format()
>               if not PatchCheckConf.ignore_change_id:
>                   self.check_change_id_format()
> +            self.check_ci_options_format()
>           self.report_message_result()
>   
>       url = 'https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format'
> @@ -324,6 +327,15 @@ class CommitMessageCheck:
>               self.error('\"%s\" found in commit message:' % cid)
>               return
>   
> +    def check_ci_options_format(self):
> +        cio='Continuous-integration-options:'
> +        for line in self.msg.splitlines():
> +            if not line.startswith(cio):
> +                continue
> +            options = line.split(':', 1)[1].split()
> +            if 'PatchCheck.ignore-multi-package' in options:
> +                self.ignore_multi_package = True
> +
>   (START, PRE_PATCH, PATCH) = range(3)
>   
>   class GitDiffCheck:
> @@ -561,6 +573,7 @@ class CheckOnePatch:
>   
>           msg_check = CommitMessageCheck(self.commit_subject, self.commit_msg, self.author_email)
>           msg_ok = msg_check.ok
> +        self.ignore_multi_package = msg_check.ignore_multi_package
>   
>           diff_ok = True
>           if self.diff is not None:
> @@ -671,6 +684,7 @@ class CheckGitCommits:
>       """
>   
>       def __init__(self, rev_spec, max_count):
> +        dec_files = self.read_dec_files_from_git()
>           commits = self.read_commit_list_from_git(rev_spec, max_count)
>           if len(commits) == 1 and Verbose.level > Verbose.ONELINE:
>               commits = [ rev_spec ]
> @@ -686,10 +700,66 @@ class CheckGitCommits:
>               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
> +            check_patch = CheckOnePatch(commit, patch)
> +            self.ok &= check_patch.ok
> +            ignore_multi_package = check_patch.ignore_multi_package
> +            if PatchCheckConf.ignore_multi_package:
> +                ignore_multi_package = True
> +            prefix = 'WARNING: ' if ignore_multi_package else ''
> +            check_parent = self.check_parent_packages (dec_files, commit, prefix)
> +            if not ignore_multi_package:
> +                self.ok &= check_parent
> +
>           if not commits:
>               print("Couldn't find commit matching: '{}'".format(rev_spec))
>   
> +    def check_parent_packages(self, dec_files, commit, prefix):
> +        ok = True
> +        modified = self.get_parent_packages (dec_files, commit, 'AM')
> +        if len (modified) > 1:
> +            print("{}The commit adds/modifies files in multiple packages:".format(prefix))
> +            print(" *", '\n * '.join(modified))
> +            ok = False
> +        deleted = self.get_parent_packages (dec_files, commit, 'D')
> +        if len (deleted) > 1:
> +            print("{}The commit deletes files from multiple packages:".format(prefix))
> +            print(" *", '\n * '.join(deleted))
> +            ok = False
> +        return ok
> +
> +    def get_parent_packages(self, dec_files, commit, filter):
> +        filelist = self.read_files_modified_from_git (commit, filter)
> +        parents = set()
> +        for file in filelist:
> +            dec_found = False
> +            for dec_file in dec_files:
> +                if os.path.commonpath([dec_file, file]):
> +                    dec_found = True
> +                    parents.add(dec_file)
> +            if not dec_found and os.path.dirname (file):
> +                # No DEC file found and file is in a subdir
> +                # Covers BaseTools, .github, .azurepipelines, .pytool
> +                parents.add(file.split('/')[0])
> +        return list(parents)
> +
> +    def read_dec_files_from_git(self):
> +        # run git ls-files *.dec
> +        out = self.run_git('ls-files', '*.dec')
> +        # return list of .dec files
> +        try:
> +            return out.split()
> +        except:
> +            return []
> +
> +    def read_files_modified_from_git(self, commit, filter):
> +        # run git diff-tree --no-commit-id --name-only -r <commit>
> +        out = self.run_git('diff-tree', '--no-commit-id', '--name-only',
> +                           '--diff-filter=' + filter, '-r', commit)
> +        try:
> +            return out.split()
> +        except:
> +            return []
> +
>       def read_commit_list_from_git(self, rev_spec, max_count):
>           # Run git to get the commit patch
>           cmd = [ 'rev-list', '--abbrev-commit', '--no-walk' ]
> @@ -800,6 +870,9 @@ class PatchCheckApp:
>           group.add_argument("--ignore-change-id",
>                              action="store_true",
>                              help="Ignore the presence of 'Change-Id:' tags in commit message")
> +        group.add_argument("--ignore-multi-package",
> +                           action="store_true",
> +                           help="Ignore if commit modifies files in multiple packages")
>           self.args = parser.parse_args()
>           if self.args.oneline:
>               Verbose.level = Verbose.ONELINE
> @@ -807,6 +880,8 @@ class PatchCheckApp:
>               Verbose.level = Verbose.SILENT
>           if self.args.ignore_change_id:
>               PatchCheckConf.ignore_change_id = True
> +        if self.args.ignore_multi_package:
> +            PatchCheckConf.ignore_multi_package = True
>   
>   if __name__ == "__main__":
>       sys.exit(PatchCheckApp().retval)


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115903): https://edk2.groups.io/g/devel/message/115903
Mute This Topic: https://groups.io/mt/104434585/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Patch 0/4] PatchCheck Updates

[Rebecca Cran]

Merged as 
dae8c29dab546fad2801e70967855a9f6ae14ae0...6d571c0070161b1b96049410f8584c0955d73536

-- 
Rebecca Cran

On 2/18/2024 1:59 PM, Michael D Kinney via groups.io wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4679
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4680
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4693
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4694
>
> Fix a collection of PatchCheck issues and add a new PatchCheck
> featue to check for a commit that modifies files in multiple
> packages with option to disable the multiple package check as
> a command line option or a commit message tag.
>
> * Reject patches that match Author email "devel@edk2.groups.io"
> * Update the current check for " via Groups.Io" to perform a
>    case insensitive match. It appears that groups.io has changed the
>    format of this string to use all lower case.
> * Update logic in CommitMessageCheck class to return errors
>    detected in commit message signatures instead of silently
>    passing the check.
> * Genenerate an error if no Cc tags are present to make sure
>    all patches Cc the required maintainers/reviewers.
>
> Cc: Rebecca Cran <rebecca@bsdio.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Bob Feng <bob.c.feng@intel.com>
> Cc: Yuwei Chen <yuwei.chen@intel.com>
> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
>
> Michael D Kinney (4):
>    BaseTools/Scripts/PatchCheck: Update Author checks
>    BaseTools/Scripts/PatchCheck: Return CommitMessageCheck errors
>    BaseTools/Scripts/PatchCheck: Error if no Cc tags are present
>    BaseTools/Scripts/PatchCheck: Error if commit modifies multiple
>      packages
>
>   BaseTools/Scripts/PatchCheck.py | 91 +++++++++++++++++++++++++++++++--
>   1 file changed, 86 insertions(+), 5 deletions(-)
>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116058): https://edk2.groups.io/g/devel/message/116058
Mute This Topic: https://groups.io/mt/104434581/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-