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

["Ard Biesheuvel" <ardb@kernel.org>]

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]
-=-=-=-=-=-=-=-=-=-=-=-