Markus Armbruster <arm...@redhat.com> writes:
> Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk> writes: > >> On 12/10/2022 13:11, Alex Bennée wrote: >> >>> Hi, >>> This is an attempt to improve our processes documentation by: >>> - adding an explicit section on maintainers >>> - reducing the up-front verbiage in patch submission >>> - emphasising the importance to respectful reviews >>> I'm sure the language could be improved further so I humbly submit >>> this RFC for discussion. >>> Alex Bennée (4): >>> docs/devel: add a maintainers section to development process >>> docs/devel: make language a little less code centric >>> docs/devel: simplify the minimal checklist >>> docs/devel: try and improve the language around patch review >>> docs/devel/code-of-conduct.rst | 2 + >>> docs/devel/index-process.rst | 1 + >>> docs/devel/maintainers.rst | 84 +++++++++++++++++++ >>> docs/devel/submitting-a-patch.rst | 101 +++++++++++++++-------- >>> docs/devel/submitting-a-pull-request.rst | 12 +-- >>> roms/qboot | 2 +- >>> 6 files changed, 157 insertions(+), 45 deletions(-) >>> create mode 100644 docs/devel/maintainers.rst >> >> Hi Alex, >> >> I've replied with a couple of things I noticed, but in general I think this >> is a really nice improvement. >> >> If you're looking at documenting some of the maintainer processes >> better, there are a few other things I have been thinking about that >> it may be worth discussing: >> >> >> i) Requiring an R-B tag for all patches before merge >> >> - Is this something we should insist on and document? >> >> ii) Unresponsive maintainers >> >> - Should we have a process for this? When Blue Swirl (the previous >> SPARC maintainer) disappeared abruptly, I think it took nearly 3 >> months to get my first patches merged >> since no-one knew if they were still active. If a maintainer has >> been unresponsive for e.g. 2 months should that then allow a process >> where other maintainers can merge >> patches on their behalf and/or start a process of maintainer transition? >> >> iii) Differences(?) between maintainers >> >> - There have been a few instances where I have been delayed in >> finding time for patch review, and in the meantime someone has >> stepped up to review the patch and given it >> an R-B tag which is great. However I have then reviewed the patch >> and noticed something amiss, and so it needs a bit more work before >> being merged. I think in >> these cases the review of the maintainer of the code in question >> should take priority over other maintainer reviews: do we need to >> explicitly document this? I can >> certainly see how this can be confusing to newcomers having an R-B >> tag as a pre-requisite for merge coming from one person, and then a >> NACK from someone else later. > > The opposite also happens: I review in my role as a maintainer, and give > my R-by, then somebody else has questions or objections. These need to > be addressed. My R-by as maintainer does not change that at all. > > Maintainers' reviews are not special. Issues raised in review need to > be addressed regardless of who raised them. Maintainers' "specialness" > kicks in at the point where they make judgement calls, such as "this is > good enough, we can address the remaining issues on top". This is my view as well - and we want to encourage as much reviewing as possible rather than implying you need to be blessed to have a view on the code. That said it is still going to be the maintainers call to take a patch through their tree and hopefully we don't have too many cases where that is subverted by going through other maintainers trees. > If multiple maintainers are responsible and disagree, they need to > hammer out some kind of consensus. Which reminds me I've quite often used the A-b tag to indicate I'm happy for a patch that touches one of my areas to go through someone elses tree. Do we actually codify that meaning anywhere? -- Alex Bennée
