Re: [RFC PATCH 0/4] docs/devel suggestions for discussion

[Markus Armbruster <armbru@redhat.com>]

Mark Cave-Ayland <mark.cave-ayland@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".

If multiple maintainers are responsible and disagree, they need to
hammer out some kind of consensus.