[PATCH net-next] docs: try to encourage (netdev?) reviewers

[PATCH net-next] docs: try to encourage (netdev?) reviewers

[Jakub Kicinski]

Add a section to netdev maintainer doc encouraging reviewers
to chime in on the mailing list.

The questions about "when is it okay to share feedback"
keep coming up (most recently at netconf) and the answer
is "pretty much always".

Extend the section of 7.AdvancedTopics.rst which deals
with reviews a little bit to add stuff we had been recommending
locally.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
--
RFC -> v1:
 - spelling (compliment)
 - move to common docs:
   - ask for more opinions
   - use of tags
   - compliments
 - ask less experienced reviewers to avoid style comments
   (using Florian's wording)

CC: andrew@lunn.ch
CC: jesse.brandeburg@intel.com
CC: sd@queasysnail.net
CC: horms@verge.net.au
CC: przemyslaw.kitszel@intel.com
CC: f.fainelli@gmail.com
CC: jiri@resnulli.us
CC: ecree.xilinx@gmail.com
---
 Documentation/process/7.AdvancedTopics.rst  | 18 ++++++++++++++++++
 Documentation/process/maintainer-netdev.rst | 15 +++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/Documentation/process/7.AdvancedTopics.rst b/Documentation/process/7.AdvancedTopics.rst
index bf7cbfb4caa5..415749feed17 100644
--- a/Documentation/process/7.AdvancedTopics.rst
+++ b/Documentation/process/7.AdvancedTopics.rst
@@ -146,6 +146,7 @@ pull.  The git request-pull command can be helpful in this regard; it will
 format the request as other developers expect, and will also check to be
 sure that you have remembered to push those changes to the public server.
 
+.. _development_advancedtopics_reviews:
 
 Reviewing patches
 -----------------
@@ -167,6 +168,12 @@ comments as questions rather than criticisms.  Asking "how does the lock
 get released in this path?" will always work better than stating "the
 locking here is wrong."
 
+Another technique useful in case of a disagreement is to ask for others
+to chime in. If a discussion reaches a stalemate after a few exchanges,
+calling for opinions of other reviewers or maintainers. Often those in
+agreement with a reviewer remain silent unless called upon.
+Opinion of multiple people carries exponentially more weight.
+
 Different developers will review code from different points of view.  Some
 are mostly concerned with coding style and whether code lines have trailing
 white space.  Others will focus primarily on whether the change implemented
@@ -176,3 +183,14 @@ security issues, duplication of code found elsewhere, adequate
 documentation, adverse effects on performance, user-space ABI changes, etc.
 All types of review, if they lead to better code going into the kernel, are
 welcome and worthwhile.
+
+There is no strict requirement to use specific tags like ``Reviewed-by``.
+In fact reviews in plain English are more informative and encouraged
+even when a tag is provided (e.g. "I looked at aspects A, B and C of this
+submission and it looks good to me.")
+Some form of a review message / reply is obviously necessary otherwise
+maintainers will not know that the reviewer has looked at the patch at all!
+
+Last but not least patch review may become a negative process, focused
+on pointing out problems. Please throw in a compliment once in a while,
+particularly for newbies!
diff --git a/Documentation/process/maintainer-netdev.rst b/Documentation/process/maintainer-netdev.rst
index 09dcf6377c27..a0cb00e7f579 100644
--- a/Documentation/process/maintainer-netdev.rst
+++ b/Documentation/process/maintainer-netdev.rst
@@ -441,6 +441,21 @@ in a way which would break what would normally be considered uAPI.
 new ``netdevsim`` features must be accompanied by selftests under
 ``tools/testing/selftests/``.
 
+Reviewer guidance
+-----------------
+
+Reviewing other people's patches on the list is highly encouraged,
+regardless of the level of expertise. For general guidance and
+helpful tips please see :ref:`development_advancedtopics_reviews`.
+
+It's safe to assume that netdev maintainers know the community and the level
+of expertise of the reviewers. The reviewers should not be concerned about
+their comments impeding or derailing the patch flow.
+
+Less experienced reviewers are highly encouraged to do more in-depth
+review of submissions and not focus exclusively on trivial / subject
+matters like code formatting, tags etc.
+
 Testimonials / feedback
 -----------------------
 
-- 
2.41.0

Re: [PATCH net-next] docs: try to encourage (netdev?) reviewers

[Donald Hunter]

Jakub Kicinski <kuba@kernel.org> writes:

> Add a section to netdev maintainer doc encouraging reviewers
> to chime in on the mailing list.
>
> The questions about "when is it okay to share feedback"
> keep coming up (most recently at netconf) and the answer
> is "pretty much always".
>
> Extend the section of 7.AdvancedTopics.rst which deals
> with reviews a little bit to add stuff we had been recommending
> locally.

I for one really appreciate this additional guidance and where better to
start than by reviewing the guidance for new reviewers :-)

Looks good other than some minor grammar nits below.

>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> --
> RFC -> v1:
>  - spelling (compliment)
>  - move to common docs:
>    - ask for more opinions
>    - use of tags
>    - compliments
>  - ask less experienced reviewers to avoid style comments
>    (using Florian's wording)
>
> CC: andrew@lunn.ch
> CC: jesse.brandeburg@intel.com
> CC: sd@queasysnail.net
> CC: horms@verge.net.au
> CC: przemyslaw.kitszel@intel.com
> CC: f.fainelli@gmail.com
> CC: jiri@resnulli.us
> CC: ecree.xilinx@gmail.com
> ---
>  Documentation/process/7.AdvancedTopics.rst  | 18 ++++++++++++++++++
>  Documentation/process/maintainer-netdev.rst | 15 +++++++++++++++
>  2 files changed, 33 insertions(+)
>
> diff --git a/Documentation/process/7.AdvancedTopics.rst b/Documentation/process/7.AdvancedTopics.rst
> index bf7cbfb4caa5..415749feed17 100644
> --- a/Documentation/process/7.AdvancedTopics.rst
> +++ b/Documentation/process/7.AdvancedTopics.rst
> @@ -146,6 +146,7 @@ pull.  The git request-pull command can be helpful in this regard; it will
>  format the request as other developers expect, and will also check to be
>  sure that you have remembered to push those changes to the public server.
>  
> +.. _development_advancedtopics_reviews:
>  
>  Reviewing patches
>  -----------------
> @@ -167,6 +168,12 @@ comments as questions rather than criticisms.  Asking "how does the lock
>  get released in this path?" will always work better than stating "the
>  locking here is wrong."
>  
> +Another technique useful in case of a disagreement is to ask for others

Another technique that is useful ...

> +to chime in. If a discussion reaches a stalemate after a few exchanges,
> +calling for opinions of other reviewers or maintainers. Often those in

then call for

> +agreement with a reviewer remain silent unless called upon.
> +Opinion of multiple people carries exponentially more weight.

The opinion

> +
>  Different developers will review code from different points of view.  Some
>  are mostly concerned with coding style and whether code lines have trailing
>  white space.  Others will focus primarily on whether the change implemented
> @@ -176,3 +183,14 @@ security issues, duplication of code found elsewhere, adequate
>  documentation, adverse effects on performance, user-space ABI changes, etc.
>  All types of review, if they lead to better code going into the kernel, are
>  welcome and worthwhile.
> +
> +There is no strict requirement to use specific tags like ``Reviewed-by``.
> +In fact reviews in plain English are more informative and encouraged
> +even when a tag is provided (e.g. "I looked at aspects A, B and C of this
> +submission and it looks good to me.")
> +Some form of a review message / reply is obviously necessary otherwise

Minor nit but I think "or" would be preferable to "/" in prose like this.

> +maintainers will not know that the reviewer has looked at the patch at all!
> +
> +Last but not least patch review may become a negative process, focused
> +on pointing out problems. Please throw in a compliment once in a while,
> +particularly for newbies!
> diff --git a/Documentation/process/maintainer-netdev.rst b/Documentation/process/maintainer-netdev.rst
> index 09dcf6377c27..a0cb00e7f579 100644
> --- a/Documentation/process/maintainer-netdev.rst
> +++ b/Documentation/process/maintainer-netdev.rst
> @@ -441,6 +441,21 @@ in a way which would break what would normally be considered uAPI.
>  new ``netdevsim`` features must be accompanied by selftests under
>  ``tools/testing/selftests/``.
>  
> +Reviewer guidance
> +-----------------
> +
> +Reviewing other people's patches on the list is highly encouraged,
> +regardless of the level of expertise. For general guidance and
> +helpful tips please see :ref:`development_advancedtopics_reviews`.
> +
> +It's safe to assume that netdev maintainers know the community and the level
> +of expertise of the reviewers. The reviewers should not be concerned about
> +their comments impeding or derailing the patch flow.
> +
> +Less experienced reviewers are highly encouraged to do more in-depth
> +review of submissions and not focus exclusively on trivial / subject

Do you mean subjective matters?

> +matters like code formatting, tags etc.
> +
>  Testimonials / feedback
>  -----------------------

Re: [PATCH net-next] docs: try to encourage (netdev?) reviewers

[Matthieu Baerts]

Hi Jakub,

On 10/10/2023 00:56, Jakub Kicinski wrote:
> Add a section to netdev maintainer doc encouraging reviewers
> to chime in on the mailing list.
> 
> The questions about "when is it okay to share feedback"
> keep coming up (most recently at netconf) and the answer
> is "pretty much always".
> 
> Extend the section of 7.AdvancedTopics.rst which deals
> with reviews a little bit to add stuff we had been recommending
> locally.

Good idea to encourage everybody to review, even the less experimented
ones. That might push me to send more reviews, even when I don't know
well the area that is being modified, thanks! :)

(...)

> diff --git a/Documentation/process/7.AdvancedTopics.rst b/Documentation/process/7.AdvancedTopics.rst
> index bf7cbfb4caa5..415749feed17 100644
> --- a/Documentation/process/7.AdvancedTopics.rst
> +++ b/Documentation/process/7.AdvancedTopics.rst
> @@ -146,6 +146,7 @@ pull.  The git request-pull command can be helpful in this regard; it will
>  format the request as other developers expect, and will also check to be
>  sure that you have remembered to push those changes to the public server.
>  
> +.. _development_advancedtopics_reviews:
>  
>  Reviewing patches
>  -----------------
> @@ -167,6 +168,12 @@ comments as questions rather than criticisms.  Asking "how does the lock
>  get released in this path?" will always work better than stating "the
>  locking here is wrong."

The paragraph just above ("it is OK to question the code") is very nice!
When I'm cced on some patches modifying some code I'm not familiar with
and there are some parts that look "strange" to me, I sometimes feel
like I only have two possibilities: either I spend quite some time
understanding that part or I give up if I don't have such time. I often
feel like I cannot say "I don't know well this part, but this looks
strange to me: are you sure it is OK to do that in such conditions?",
especially when the audience is large and/or the author of the patch is
an experienced developer.

> +Another technique useful in case of a disagreement is to ask for others
> +to chime in. If a discussion reaches a stalemate after a few exchanges,
> +calling for opinions of other reviewers or maintainers. Often those in
> +agreement with a reviewer remain silent unless called upon.
> +Opinion of multiple people carries exponentially more weight.
> +
>  Different developers will review code from different points of view.  Some
>  are mostly concerned with coding style and whether code lines have trailing
>  white space.  Others will focus primarily on whether the change implemented
> @@ -176,3 +183,14 @@ security issues, duplication of code found elsewhere, adequate
>  documentation, adverse effects on performance, user-space ABI changes, etc.
>  All types of review, if they lead to better code going into the kernel, are
>  welcome and worthwhile.
> +
> +There is no strict requirement to use specific tags like ``Reviewed-by``.
> +In fact reviews in plain English are more informative and encouraged
> +even when a tag is provided (e.g. "I looked at aspects A, B and C of this
> +submission and it looks good to me.")

That's a very good point, good idea to insist on that!

Small nit: if I'm not mistaken, in reStructuredText, if there is no
blank line in between the lines, the text will be merged in the rendered
output and the previous line is not ending with a dot :)

Also, personally, I would have removed the parentheses, but I'm not sure
what are the rules about that in English:

  In fact <...> when a tag is provided, e.g. "I looked at <...>".

> +Some form of a review message / reply is obviously necessary otherwise
> +maintainers will not know that the reviewer has looked at the patch at all!
> +
> +Last but not least patch review may become a negative process, focused
> +on pointing out problems. Please throw in a compliment once in a while,
> +particularly for newbies!

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH net-next] docs: try to encourage (netdev?) reviewers

[Geert Uytterhoeven]

Hi Matt,

On Tue, Oct 10, 2023 at 5:19 PM Matthieu Baerts <matttbe@kernel.org> wrote:
> On 10/10/2023 00:56, Jakub Kicinski wrote:
> > Add a section to netdev maintainer doc encouraging reviewers
> > to chime in on the mailing list.
> >
> > The questions about "when is it okay to share feedback"
> > keep coming up (most recently at netconf) and the answer
> > is "pretty much always".
> >
> > Extend the section of 7.AdvancedTopics.rst which deals
> > with reviews a little bit to add stuff we had been recommending
> > locally.
>
> Good idea to encourage everybody to review, even the less experimented
> ones. That might push me to send more reviews, even when I don't know
> well the area that is being modified, thanks! :)
>
> (...)
>
> > diff --git a/Documentation/process/7.AdvancedTopics.rst b/Documentation/process/7.AdvancedTopics.rst
> > index bf7cbfb4caa5..415749feed17 100644
> > --- a/Documentation/process/7.AdvancedTopics.rst
> > +++ b/Documentation/process/7.AdvancedTopics.rst
> > @@ -146,6 +146,7 @@ pull.  The git request-pull command can be helpful in this regard; it will
> >  format the request as other developers expect, and will also check to be
> >  sure that you have remembered to push those changes to the public server.
> >
> > +.. _development_advancedtopics_reviews:
> >
> >  Reviewing patches
> >  -----------------
> > @@ -167,6 +168,12 @@ comments as questions rather than criticisms.  Asking "how does the lock
> >  get released in this path?" will always work better than stating "the
> >  locking here is wrong."
>
> The paragraph just above ("it is OK to question the code") is very nice!
> When I'm cced on some patches modifying some code I'm not familiar with
> and there are some parts that look "strange" to me, I sometimes feel
> like I only have two possibilities: either I spend quite some time
> understanding that part or I give up if I don't have such time. I often
> feel like I cannot say "I don't know well this part, but this looks
> strange to me: are you sure it is OK to do that in such conditions?",
> especially when the audience is large and/or the author of the patch is
> an experienced developer.

Yes you can (even experienced developers can make mistakes ;-)!

If it is not obvious that something is safe, it is better to point it
out, so the submitter (or someone else) can give it a (second) thought.
In case it is safe, and you didn't miss the ball completely, it probably
warrants a comment in the code, or an improved patch description.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH net-next] docs: try to encourage (netdev?) reviewers

[Matthieu Baerts]

Hi Geert,

On 10/10/2023 17:52, Geert Uytterhoeven wrote:
> Hi Matt,
> 
> On Tue, Oct 10, 2023 at 5:19 PM Matthieu Baerts <matttbe@kernel.org> wrote:
>> On 10/10/2023 00:56, Jakub Kicinski wrote:
>>> Add a section to netdev maintainer doc encouraging reviewers
>>> to chime in on the mailing list.
>>>
>>> The questions about "when is it okay to share feedback"
>>> keep coming up (most recently at netconf) and the answer
>>> is "pretty much always".
>>>
>>> Extend the section of 7.AdvancedTopics.rst which deals
>>> with reviews a little bit to add stuff we had been recommending
>>> locally.
>>
>> Good idea to encourage everybody to review, even the less experimented
>> ones. That might push me to send more reviews, even when I don't know
>> well the area that is being modified, thanks! :)
>>
>> (...)
>>
>>> diff --git a/Documentation/process/7.AdvancedTopics.rst b/Documentation/process/7.AdvancedTopics.rst
>>> index bf7cbfb4caa5..415749feed17 100644
>>> --- a/Documentation/process/7.AdvancedTopics.rst
>>> +++ b/Documentation/process/7.AdvancedTopics.rst
>>> @@ -146,6 +146,7 @@ pull.  The git request-pull command can be helpful in this regard; it will
>>>  format the request as other developers expect, and will also check to be
>>>  sure that you have remembered to push those changes to the public server.
>>>
>>> +.. _development_advancedtopics_reviews:
>>>
>>>  Reviewing patches
>>>  -----------------
>>> @@ -167,6 +168,12 @@ comments as questions rather than criticisms.  Asking "how does the lock
>>>  get released in this path?" will always work better than stating "the
>>>  locking here is wrong."
>>
>> The paragraph just above ("it is OK to question the code") is very nice!
>> When I'm cced on some patches modifying some code I'm not familiar with
>> and there are some parts that look "strange" to me, I sometimes feel
>> like I only have two possibilities: either I spend quite some time
>> understanding that part or I give up if I don't have such time. I often
>> feel like I cannot say "I don't know well this part, but this looks
>> strange to me: are you sure it is OK to do that in such conditions?",
>> especially when the audience is large and/or the author of the patch is
>> an experienced developer.
> 
> Yes you can (even experienced developers can make mistakes ;-)!

Thank you for your reply!

> If it is not obvious that something is safe, it is better to point it
> out, so the submitter (or someone else) can give it a (second) thought.
> In case it is safe, and you didn't miss the ball completely, it probably
> warrants a comment in the code, or an improved patch description.

Indeed, good point!

It is good then to have that written in the doc -- I only discovered it
recently -- because, at least for me, it is easy to think that
experienced developers never make mistakes ( ;) ) and questioning their
code can only be done if we have double or triple checked that there is
likely an issue :)

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net