[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 10 Oct 2023 09:41:33 +0100
From: Donald Hunter <donald.hunter@...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] docs: try to encourage (netdev?) reviewers
Jakub Kicinski <kuba@...nel.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@...nel.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@...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..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
> -----------------------
Powered by blists - more mailing lists