From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id A2DFDAC1B53 for ; Wed, 21 Feb 2024 22:16:17 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=N6ySnFjvbzxftwttS/oKb6SSsqCPtuKmng6C/mnCSDo=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1708553776; v=1; b=gAkN86faKSbHj4bjyTzD2wX2iJLaKsMfmhgt0Ib/Wvn6L2hTVMPqQ78YLkbwdkWzNz1mSOMI m1w+Dj8yXiRpo3iigI2T9PLMdQ+x3dfEuZikW5dW74tMKQPZDi0WrjGqgWEpPbVGhp9WiyztD2y 0ZXws2K4+hePwlUvYpYlsGro= X-Received: by 127.0.0.2 with SMTP id V1NjYY7687511xChTse43aXh; Wed, 21 Feb 2024 14:16:16 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web11.1629.1708553775763375917 for ; Wed, 21 Feb 2024 14:16:15 -0800 X-Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-300-iZDMLUKgN-20MlmBEA47Wg-1; Wed, 21 Feb 2024 17:16:11 -0500 X-MC-Unique: iZDMLUKgN-20MlmBEA47Wg-1 X-Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 9C9C729AA382; Wed, 21 Feb 2024 22:16:09 +0000 (UTC) X-Received: from [10.39.192.46] (unknown [10.39.192.46]) by smtp.corp.redhat.com (Postfix) with ESMTPS id C1E62112132A; Wed, 21 Feb 2024 22:16:07 +0000 (UTC) Message-ID: Date: Wed, 21 Feb 2024 23:16:06 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [Patch 3/4] BaseTools/Scripts/PatchCheck: Error if no Cc tags are present To: devel@edk2.groups.io, ardb@kernel.org, michael.d.kinney@intel.com Cc: Rebecca Cran , Liming Gao , Bob Feng , Yuwei Chen , Michael Kubacki References: <20240218205951.497-1-michael.d.kinney@intel.com> <20240218205951.497-4-michael.d.kinney@intel.com> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.3 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: xOQ6lrmHqySiOZ19ZOrbOORCx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=gAkN86fa; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On 2/20/24 15:48, Ard Biesheuvel wrote: > Hello Mike, >=20 > I understand the desire to be pedantic about cc'ing the right > maintainers, but I'm not convinced this is the way. >=20 > - 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, >=20 > - 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. >=20 > 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.) >=20 > 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 >=20 >=20 > On Sun, 18 Feb 2024 at 22:00, Michael D Kinney > wrote: >> >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D4694 >> >> 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 >> Cc: Liming Gao >> Cc: Bob Feng >> Cc: Yuwei Chen >> Cc: Michael Kubacki >> Signed-off-by: Michael D Kinney >> --- >> BaseTools/Scripts/PatchCheck.py | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCh= eck.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 =3D self.find_signatures(sigtype) >> + if sigtype =3D=3D 'Cc' and len(sigs) =3D=3D 0: >> + self.error('No Cc: tags for maintainers/reviewers found= !') >> >> cve_re =3D re.compile('CVE-[0-9]{4}-[0-9]{5}[^0-9]') >> >> -- >> 2.40.1.windows.1 >> >> >> >> >> >> >=20 >=20 >=20 >=20 >=20 -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- 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] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-