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:0.120 / U:2.280 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site