Hi Matt,
On Tue, Oct 10, 2023 at 5:19 PM Matthieu Baerts <matttbe@xxxxxxxxxx> 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 ;-)!
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@xxxxxxxxxxxxxx
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
