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

Reply via email to