On Thu, Oct 01, 2020 at 10:44:10 +0200, Laszlo Ersek wrote:
On 09/30/20 12:13, Leif Lindholm wrote:
Reviever or Maintainer can approve a patch. Any Maintainer can push a
patch that has been approved.
Assume Ard and myself are away and Jordan fails to report back in a week
or so, but Rebecca or Peter have reviewed a patch on the list for
In that case, the patch should *NOT* be merged by (for example) you,
just because you have push rights. The community will have to wait until
Ard, Jordan, or myself return and provide an ACK.
If the maintainers are *consistently* irresponsive, then new maintainers
need to be added, possibly with a larger community discussion. But if
it's just a week (especially if we discussed our absence in advance),
then maintainer absence is completely sufficient and justified for
holding back patches, even if designated reviewers are OK with those
I've been *really* disliking that, for example, the chief MdeModulePkg
reviewers don't regularly ACK patches that have been reviewed by
designated reviewers. If those reviewers are considered authoritative
enough to fully approve patches -- and most of them they have push
access already, anyway --, then we should rework Maintainers.txt so that
Maintainer roles be handed out with a finer granularity. If you will:
promote those reviewers to Maintainers, on their respective turfs.
This can happen either:I don't see it like that; I think Maintainers should have the last word
- when the designated Maintainer for that patch is
- if the patch submitter is also a Maintainer of some other part of
No one can approve their own patches.
The act of adding a Reviewer means delegating the approval work to
on every patch going in. And yes, this *requires* maintainers to be
... Hm. Perhaps this is a sign that we really have two concepts here,
we've just not been distinguishing them clearly enough. Maybe we need to
split the reviewer role in two: "Approving Reviewer" and "Assistant
I think you're right. This is why we seem to have two sets of opinions
on this topic.
For example, on OvmfPkg, I would suggest marking all current Reviewers
as "Assistant Reviewers". On ArmVirtPkg, I'd likely propose you as an
Approving Reviewer (you have stood in for Ard and myself anyway, for
years now), and suggest Assistant Reviewer role for Julien.
Right, that makes sense to me.
If I was to start bikeshedding, I might suggest adding an A: tag for
approving reviewer. Possibly stealing the description from the current
R: tag, and adding the approving bit. And maybe nicking the QEMU R:
description outright for R:.
MdeModulePkg and other core packages, I'd defer the re-classification to
Intel; we'd likely see a really large number of Approving Reviewers
(justifiedly, I think).