linuxppc-dev.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
From: Daniel Axtens <dja@axtens.net>
To: Michael Ellerman <mpe@ellerman.id.au>,
	Denis Kirjanov <kda@linux-powerpc.org>
Cc: linuxppc-dev <linuxppc-dev@lists.ozlabs.org>
Subject: Re: linuxppc patchwork queue
Date: Tue, 24 Nov 2015 08:56:54 +1100	[thread overview]
Message-ID: <87r3jgv0pl.fsf@gamma.ozlabs.ibm.com> (raw)
In-Reply-To: <1448273552.3452.7.camel@ellerman.id.au>

[-- Attachment #1: Type: text/plain, Size: 1865 bytes --]

> The main thing you could do, and anyone could do (!), is just review some
> patches. Even if you don't know the area of code that well, you can usually do
> a basic review.
>
> eg. Just the basic stuff:
>  - Is the subject correctly formatted and makes sense.
>  - Is the change log well written and describes the change well.
>  - Does the patch do one logical change.
>  - Are there any obviously bad coding style things.
>  - Can you follow the basics of the change based on the change log and some
>    reading of the code?

Michael gave me the same advice earlier this year, and I found it really
helpful. As a result, I've been trying to do reviews.

I am by no means an expert, but here's what I found starting out:

 - Reviewing is a learned skill - you don't just start doing kernel dev
   and get this magical sense of how to do reviews. This means it's OK
   to start small, and that no-one expects your early reviews to be
   amazing.

 - Reviewing gets much easier when you just jump in and start doing it!

 - A review doesn't have to focus on getting to a reviewed-by
   tag. Asking good questions is equally if not more valuable.

 - It was easier to start with the 'little picture', or what Michael
   calls the 'basic stuff'. Is it good C? Does the commit message make
   sense? Is the spelling right? Does it do weird things without
   explaining them? When you're starting it's OK to leave the 'big
   picture'/architectural/design reviews to other people. I definitely
   still do!

HTH.
Daniel

>
> Another thing I seem to spend a lot of time on is asking people if their patch
> should go to stable or not, and if so what versions. So everyone could help out
> there.
>
> cheers
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 859 bytes --]

  reply	other threads:[~2015-11-23 21:57 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-11-20 14:48 linuxppc patchwork queue Denis Kirjanov
2015-11-23  0:15 ` Michael Ellerman
2015-11-23  9:45   ` Denis Kirjanov
2015-11-23 10:12     ` Michael Ellerman
2015-11-23 21:56       ` Daniel Axtens [this message]
2015-11-26  6:49         ` Michael Ellerman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87r3jgv0pl.fsf@gamma.ozlabs.ibm.com \
    --to=dja@axtens.net \
    --cc=kda@linux-powerpc.org \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=mpe@ellerman.id.au \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).