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

Hi Matt,

On Tue, Oct 10, 2023 at 5:19 PM Matthieu Baerts [off-list ref] wrote:

On 10/10/2023 00:56, Jakub Kicinski wrote:

quoted

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! :)

(...)

quoted

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.

Yes you can (even experienced developers can make mistakes ;-)!

If it is not obvious that something is safe, it is better to point it
out, so the submitter (or someone else) can give it a (second) thought.
In case it is safe, and you didn't miss the ball completely, it probably
warrants a comment in the code, or an improved patch description.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds