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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>

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

> --
> v2:
>  - grammar fixes from Donald
>  - remove parenthesis around a quote
> v1: https://lore.kernel.org/all/20231009225637.3785359-1-kuba@xxxxxxxxxx/
>  - 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@xxxxxxx
> CC: jesse.brandeburg@xxxxxxxxx
> CC: sd@xxxxxxxxxxxxxxx
> CC: horms@xxxxxxxxxxxx
> CC: przemyslaw.kitszel@xxxxxxxxx
> CC: f.fainelli@xxxxxxxxx
> CC: jiri@xxxxxxxxxxx
> CC: ecree.xilinx@xxxxxxxxx
> ---
>  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
> 




[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux