From 6e55b1cbf05dca4651692be6c59acb7b20186653 Mon Sep 17 00:00:00 2001 From: Jakub Kicinski Date: Tue, 10 Oct 2023 19:42:24 -0700 Subject: docs: try to encourage (netdev?) reviewers 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 Reviewed-by: Florian Fainelli Reviewed-by: Martin Habets Signed-off-by: David S. Miller --- Documentation/process/7.AdvancedTopics.rst | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) (limited to 'Documentation/process/7.AdvancedTopics.rst') 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! -- cgit v1.2.3