| Message ID | 20221012121152.1179051-1-alex.bennee@linaro.org |
|---|---|
| Headers | show |
| Series | docs/devel suggestions for discussion | expand |
On Wed, Oct 12, 2022 at 01:11:48PM +0100, 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 > > -- > 2.34.1 > Modulo comments: Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
On 10/12/22 14: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 Thanks, these are useful improvements. On top we could probably merge some content from Linux and make the documentation standalone. But still: Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> after addressing comments from Stefan and myself. Paolo
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. ATB, Mark.
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.
Markus Armbruster <armbru@redhat.com> writes: > 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". 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?