On Wed, 21 Feb 2024 at 23:16, Laszlo Ersek <ler...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
