Hi Geert,

On 10/10/2023 17:52, Geert Uytterhoeven wrote:

Hi Matt,

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

quoted

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

Thank you for your reply!

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.

Indeed, good point!

It is good then to have that written in the doc -- I only discovered it
recently -- because, at least for me, it is easy to think that
experienced developers never make mistakes ( ;) ) and questioning their
code can only be done if we have double or triple checked that there is
likely an issue :)

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net