Date: Tue, 10 Oct 2023 17:19:38 +0200
From: Matthieu Baerts <matttbe@...nel.org>
To: Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net
Cc: netdev@...r.kernel.org, edumazet@...gle.com, pabeni@...hat.com,
 corbet@....net, workflows@...r.kernel.org, linux-doc@...r.kernel.org,
 andrew@...n.ch, jesse.brandeburg@...el.com, sd@...asysnail.net,
 horms@...ge.net.au, przemyslaw.kitszel@...el.com, f.fainelli@...il.com,
 jiri@...nulli.us, ecree.xilinx@...il.com
Subject: Re: [PATCH net-next] docs: try to encourage (netdev?) reviewers

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

Powered by blists - more mailing lists