lkml.org 
[lkml]   [2013]   [May]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 1/8] Documentation: Adding "The Perfect Patch" by Andrew Morton
    On 05/23/2013 07:49:53 AM, Ben Minerds wrote:
    > Adding Andrews advice on patch submission and subdirectory for
    > further patch
    > documentstion.

    You've seen Documentation/SubmittingPatches right?

    > Signed-off-by: Ben Minerds <puzzleduck@gmail.com>
    > ---
    > .../patches/The-Perfect-Patch.txt | 167
    > +++++++++++++++++++++
    > 1 file changed, 167 insertions(+)
    > create mode 100644
    > Documentation/development-process/patches/The-Perfect-Patch.txt
    >
    > diff --git
    > a/Documentation/development-process/patches/The-Perfect-Patch.txt
    > b/Documentation/development-process/patches/The-Perfect-Patch.txt
    > new file mode 100644
    > index 0000000..b07074e
    > --- /dev/null
    > +++ b/Documentation/development-process/patches/The-Perfect-Patch.txt
    > @@ -0,0 +1,167 @@
    > +From: Andrew Morton [email blocked]
    > +To: Mukker, Atul [email blocked]
    > +Subject: Re: [patch] 2.6.9-rc1-mm1: megaraid_mbox.c compile error
    > with gcc 3.4
    > +Date: Sat, 28 Aug 2004 13:04:19 -0700
    > +

    This email is eight years old.

    > +"Mukker, Atul" [email blocked] wrote:
    > +>
    > +> The driver and the patches with the re-ordered functions is
    > available at
    > +> ftp://ftp.lsil.com/pub/linux-megaraid/drivers/version-2.20.3.1/
    > +
    > +I dunno about James, but I *really* dislike receiving patches by
    > going and
    > +getting them from internet servers. It breaks our commonly-used
    > tools. It
    > +loses authorship info. It loses Signed-off-by: info. There is no
    > +changelog. All this means that your patch is more likely to be
    > ignored by
    > +busy people. Please, just email the diffs.
    > +
    > +I wrote a little guide this week:
    > +
    > +
    > +
    > +The perfect patch. [email blocked]
    > +
    > +The latest version of this document may be found at
    > +http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt

    Could you just grab the actual version from his repo instead of one
    that says "email blocked" and commemorates the URL of a rejected
    submission in a chatty email header?

    > +Delivery
    > +========
    > +
    > +- Patches are delivered via email only. Downloading them from
    > internet
    > + servers is a pain.
    > +
    > +- One patch per email, with the changelog in the body of the email.
    > +
    > +Subject:
    > +========
    > +
    > +- The email's Subject: should consisely describe the patch which
    > that email
    > + contains. The Subject: should not be a filename. Do not use the
    > same
    > + Subject: for every patch in a whole patch series.
    > +
    > + Bear in mind that the Subject: of your email becomes a
    > globally-unique
    > + identifier for that patch. It propagates all the way into
    > BitKeeper. The
    > + Subject: may later be used in developer discussions which refer to
    > the
    > + patch. People will want to google for the patch's Subject: to read
    > + discussion regarding that patch.
    > +
    > +- When sending a series of patches, the patches should be
    > sequence-numbered
    > + in the Subject:
    > +
    > +- It is nice if the Subject: includes mention of the subsystem which
    > it
    > + affects. See example below.
    > +
    > +- Example Subject:
    > +
    > + [patch 2/5] ext2: improve scalability of bitmap searching
    > +
    > +- Note that various people's patch receiving scripts will strip away
    > + Subject: text which is inside brackets []. So you should place
    > information
    > + which has no long-term relevance to the patch inside brackets.
    > This
    > + includes the word "patch" and any sequence numbering. The
    > subsystem
    > + identifier ("ext2:") should hence be outside brackets.
    > +
    > +
    > +Changelog
    > +=========
    > +
    > +- Bear in mind that the Subject: and changelog which you provide will
    > + propagate all the way into the permanent kernel record. Other
    > developers
    > + will want to read and understand your patch and changelog years in
    > the
    > + future.
    > +
    > + So the changelog should describe the patch fully:
    > +
    > + - why the kernel needed patching
    > +
    > + - the overall design approach in the patch
    > +
    > + - implementation details
    > +
    > + - testing results
    > +
    > +- Don't bother mentioning what version of the kernel the patch
    > applies to
    > + ("applies to 2.6.8-rc1"). This is not interesting information -
    > once the
    > + patch is in bitkeeper, of _course_ it applied, and it'll probably
    > be merged
    > + into a later kernel than the one which you wrote it for.
    > +

    Bitkeeper?

    Is this really current advice? What part of this has _not_ been put in
    SubmittingPatches yet?

    (I'm all for moving SubmittingPatches and friends under
    development-process/ and in fact vaguely plan a general Documentation/
    tree cleanup next time I have some downtime between contracts. But
    adding eight year old duplication: not so much.)

    > +- Do not refer to earlier patches when changelogging a new version
    > of a
    > + patch. It's not very useful to have a bitkeeper changelog which
    > says "OK,
    > + this fixes the things you mentioned yesterday". Each iteration of
    > the patch
    > + should contain a standalone changelog. This implies that you need
    > a patch
    > + management system which maintains changelogs. See below.
    > +
    > +- Add a Signed-off-by: line.
    > +

    There's a whole edifice of signed-off-by line advice elsewhere in
    Documentation. (The pointy-haired types descended on this and attempted
    to make a bureaucracy out of it.)

    > +- Most people's patch receiving scripts will treat a ^--- string as
    > the
    > + separator between the changelog and the patch itself. You can use
    > this to
    > + ensure that any diffstat information is discarded when the patch
    > is applied:
    > +
    > +
    > +
    > + Another few #if/#ifdef cleanups, this time for the PPC
    > architecture.
    > +
    > + Signed-off-by: <valdis.kletnieks@vt.edu>
    > + Signed-off-by: Andrew Morton [email blocked]
    > + ---
    > +
    > + 25-akpm/arch/ppc/kernel/process.c | 2 +-
    > + 25-akpm/arch/ppc/platforms/85xx/mpc85xx_cds_common.c | 2 +-
    > + 25-akpm/arch/ppc/syslib/ppc85xx_setup.c | 4
    > ++--
    > + 3 files changed, 4 insertions(+), 4 deletions(-)
    > +
    > + --- 25/arch/ppc/kernel/process.c
    > + +++ 25/arch/ppc/kernel/process.c
    > + @@ -667,7 +667,7 @@ void show_stack(struct task_struct *tsk,
    > +

    SubmittingPatches has recommended diffstat command lines...

    > +
    > +The diff
    > +========
    > +
    > +- Patches should be in `patch -p1' form:
    > +
    > + --- a/kernel/sched.c
    > + +++ b/kernel/sched.c
    > +
    > +- Make sure that your patches apply to the latest version of the
    > kernel
    > + tree. Either straight from bitkeeper or from
    > + ftp://ftp.kernel.org/pub/linux/kernel/v2.6/snapshots/
    > +
    > +- When raising patches for -mm it's generally best to base them on
    > the
    > + latest Linus tree. I'll work out any rejects/incompatibilities.
    > There are
    > + of course exceptions to this.
    > +
    > +- Ensure that your patch does not add new trailing whitespace. The
    > below
    > + script will fix up your patch by stripping off such whitespace.
    > +
    > + #!/bin/sh
    > +
    > + strip1()
    > + {
    > + TMP=$(mktemp /tmp/XXXXXX)
    > + cp $1 $TMP
    > + sed -e '/^+/s/[ ]*$//' < $TMP > $1
    > + rm $TMP
    > + }
    > +
    > + for i in $*
    > + do
    > + strip1 $i
    > + done

    Doesn't scripts/checkpatch.pl check for this?

    > +
    > +Overall
    > +=======
    > +
    > +- Avoid MIME and attachements if possible. Make sure that your
    > email client
    > + does not wordwrap your patch. Make sure that your email client
    > does not
    > + replace tabs with spaces.

    We have Docuemntation/email-clients.txt which would probably also go
    under development-process/.

    Rob

    \
     
     \ /
      Last update: 2013-05-24 07:01    [W:2.162 / U:0.056 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site