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

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geert,

On 10/10/2023 17:52, Geert Uytterhoeven wrote:
> Hi Matt,
> 
> On Tue, Oct 10, 2023 at 5:19 PM Matthieu Baerts <matttbe@kernel.org> wrote:
>> On 10/10/2023 00:56, 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.
>>
>> 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! :)
>>
>> (...)
>>
>>> 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