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 <a...@kernel.org> > Sent: Tuesday, February 20, 2024 6:49 AM > To: devel@edk2.groups.io; Kinney, Michael D > <michael.d.kin...@intel.com> > Cc: Rebecca Cran <rebe...@bsdio.com>; Liming Gao > <gaolim...@byosoft.com.cn>; Feng, Bob C <bob.c.f...@intel.com>; Chen, > Christine <yuwei.c...@intel.com>; Michael Kubacki > <mikub...@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.kin...@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 <rebe...@bsdio.com> > > Cc: Liming Gao <gaolim...@byosoft.com.cn> > > Cc: Bob Feng <bob.c.f...@intel.com> > > Cc: Yuwei Chen <yuwei.c...@intel.com> > > Cc: Michael Kubacki <mikub...@linux.microsoft.com> > > Signed-off-by: Michael D Kinney <michael.d.kin...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
