Date: Thu, 12 Oct 2023 08:55:50 +0100
From: Martin Habets <habetsm.xilinx@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, 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 v2] docs: try to encourage (netdev?) reviewers

On Tue, Oct 10, 2023 at 07:42:24PM -0700, 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.
> 
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>

Reviewed-by: Martin Habets <habetsm.xilinx@...il.com>

> --
> v2:
>  - grammar fixes from Donald
>  - remove parenthesis around a quote
> v1: https://lore.kernel.org/all/20231009225637.3785359-1-kuba@kernel.org/
>  - 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@...n.ch
> CC: jesse.brandeburg@...el.com
> CC: sd@...asysnail.net
> CC: horms@...ge.net.au
> CC: przemyslaw.kitszel@...el.com
> CC: f.fainelli@...il.com
> CC: jiri@...nulli.us
> CC: ecree.xilinx@...il.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..43291704338e 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 that is useful in case of a disagreement is to ask for others
> +to chime in. If a discussion reaches a stalemate after a few exchanges,
> +then call for opinions of other reviewers or maintainers. Often those in
> +agreement with a reviewer remain silent unless called upon.
> +The 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 or 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..7feacc20835e 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 or subjective
> +matters like code formatting, tags etc.
> +
>  Testimonials / feedback
>  -----------------------
>  
> -- 
> 2.41.0
> 

Powered by blists - more mailing lists