Help with patch formatting

[Randy Dunlap]

On Tue, 16 Oct 2007 12:10:58 +1000 Gilles Gigan wrote:

> Hi,
> I submitted my patch to LKML and was asked to resubmit "a new version of this patch which has a changelog, isn't
> space-stuffed and isn't wordwrapped?" [akpm]
> I m not sure why I was asked this. Looking at my last submission
> (http://selenic.com/pipermail/kernel-mentors/2007-October/000672.html):
> 
> - changelog: reading "The perfect patch" again, shows what s expected in a Changelog, so i ll fix that.
> 
> - space-stuffing: AFAIK, there are no trailing spaces at the end of lines, the only extra spaces I can see were
> introduced when I ran scripts/Lindent on the source file. Long lines are split over multiple lines and these chunks are
> indented with a couple of tabs and multiple spaces. For example:
> 	if (t < 1 || t > WATCHDOG_MAX_TIMEOUT) {
> 		printk(KERN_ERR PREFIX "timeout value must be 1<=x<=%d\n", WATCHDOG_MAX_TIMEOUT);
> becomes
> 	if (t < 1 || t > WATCHDOG_MAX_TIMEOUT) {
> 		printk(KERN_ERR PREFIX "timeout value must be 1<=x<=%d\n",
> 		       WATCHDOG_MAX_TIMEOUT);
> after running Lindent. As you can see, the third line has 2 tabs and 7 spaces ! If this is the "spacestuffing" Andrew is
> talking about, what shall I do? Am I expected to go, review and fix the code produced by Lindent ?

I didn't see the space-stuffing, but Lindent is known to muck things
up sometimes, especially on labels:.  So yes, anywhere that you see
things like the example that you gave above, or you see labels: that
are indented, fix them.  We indent labels either not at all or only
1 space (and the latter just because some versions of 'patch' don't
handle unindented labels correctly).

Also run your patch thru checkpatch.pl (get the latest version from
http://www.kernel.org/pub/linux/kernel/people/apw/checkpatch/ ) and
read its suggestions (but some of them may be a bit over the edge
in some cases, i.e., not all of them are strict must-change items).


> - wordwrapping: Lines are supposed to be no more than 80 chars. Again, the only thing I can see is lines broken into
> several chunks by Lindent. Same as before, if this is the "wordwrapping" Andrew is talking about, what shall I do? Am I
> expected to go, review and fix the code produced by Lindent ?

Yes, Lindent can screw things up.  :(

> the question is : Is Lindent at fault ? Do people use Lindent at all ? Being completely new at submitting code to the
> LKML, I just want to make sure i get things right from the beginning. I did read several documents on that topic
> (CodingStyle, SubmitChecklist, SubmittingPatches, Submittingdrivers, "the perfect patch" from Andrew himself).
> Codingstyle actually says "... or use scripts/Lindent, which indents in the latest style". BTW, I also noticed that
> indents changes
> static ssize_t fop_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
> into
> static ssize_t fop_write(struct file *file, const char __user * buf,
> 			 size_t count, loff_t * ppos)
> there is now an extra space between the star (*) for buf and ppos, which according to CodingStyle is bad.

Bad Lindent.  No treat.

> Below is the patch I would like to resubmit. I went through it and made changes by hand.
> Comments are welcome.

The (updated) patch should apply to 2.6.23-git<latest> cleanly.
(It does not.)

1 out of 1 hunk FAILED -- saving rejects to file drivers/char/watchdog/Kconfig.rej
1 out of 1 hunk FAILED -- saving rejects to file drivers/char/watchdog/Makefile.rej

checkpatch.pl-next only complains about the indented labels.

No Subject: line in the patch body.  Begin with "From:" line.

Signed-off-by: Gilles Gigan <gilles.gigan at jcu.edu.au

 ................................................. add > up there^


There are some long lines that are being split/broken for you...
this is bad.  See most (or all) of the "diff options filename1 filename2"
lines.  They come out as 2 lines in the received email.
This is usually an email client problem.  Read
http://mbligh.org/linuxdocs/Email/Clients/Thunderbird.  You may
need to use the tbird External Editor plugin.  It works pretty well
in my experience.  (The rest of the patch lines loook OK to me
as far as being split is concerned.)

This line:
+ *      10/01- 2007   [Initial revision]

use spaces instead of a tab to get to the date column.
OTOH, that type of comment is SCM metadata material (but implicitly
obvious in the SCM metadata in this case).  Please drop that line
and don't put change history in the source file.  It goes into
the changelog & SCM metadata.

The rest of the patch looks OK to me.


> Subject: [PATCH] watchdog: add Nano 7240 driver
> 
> from: Gilles Gigan <gilles.gigan at bluebottle.com>
> 
> Adds support for the built-in watchdog on EPIC Nano 7240 boards from IEI.
> 
> Tested on Nano-7240RS.
> 
> Hardware documentation can be found on the IEI website: http://www.ieiworld.com
> 
> Signed-off-by: Gilles Gigan <gilles.gigan at jcu.edu.au
> ---
> diff -uprN -X linux-2.6.23-rc9/Documentation/dontdiff linux-2.6.23-rc9/drivers/char/watchdog/Kconfig
> linux-2.6.23-rc9-dirty/drivers/char/watchdog/Kconfig
> --- linux-2.6.23-rc9/drivers/char/watchdog/Kconfig	2007-10-06 01:43:44.000000000 +1000
> +++ linux-2.6.23-rc9-dirty/drivers/char/watchdog/Kconfig	2007-10-16 11:29:15.000000000 +1000
> @@ -455,6 +455,19 @@ config SBC8360_WDT
> 
>   	  Most people will say N.
> 
> +config SBC7240_WDT
> +	tristate "SBC Nano 7240 Watchdog Timer"
> +	depends on X86_32
> +	---help---
> +	  This is the driver for the hardware watchdog found on the IEI
> +	  single board computers EPIC Nano 7240 (and likely others). This
> +	  watchdog simply watches your kernel to make sure it doesn't freeze,
> +	  and if it does, it reboots your computer after a certain amount of
> +	  time.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called sbc7240_wdt.
> +
>   config CPU5_WDT
>   	tristate "SMA CPU5 Watchdog"
>   	depends on X86
> diff -uprN -X linux-2.6.23-rc9/Documentation/dontdiff linux-2.6.23-rc9/drivers/char/watchdog/Makefile
> linux-2.6.23-rc9-dirty/drivers/char/watchdog/Makefile
> --- linux-2.6.23-rc9/drivers/char/watchdog/Makefile	2007-10-06 01:43:44.000000000 +1000
> +++ linux-2.6.23-rc9-dirty/drivers/char/watchdog/Makefile	2007-10-16 11:29:15.000000000 +1000
> @@ -71,6 +71,7 @@ obj-$(CONFIG_SCx200_WDT) += scx200_wdt.o
>   obj-$(CONFIG_PC87413_WDT) += pc87413_wdt.o
>   obj-$(CONFIG_60XX_WDT) += sbc60xxwdt.o
>   obj-$(CONFIG_SBC8360_WDT) += sbc8360.o
> +obj-$(CONFIG_SBC7240_WDT) += sbc7240_wdt.o
>   obj-$(CONFIG_CPU5_WDT) += cpu5wdt.o
>   obj-$(CONFIG_SMSC37B787_WDT) += smsc37b787_wdt.o
>   obj-$(CONFIG_W83627HF_WDT) += w83627hf_wdt.o
> diff -uprN -X linux-2.6.23-rc9/Documentation/dontdiff linux-2.6.23-rc9/drivers/char/watchdog/sbc7240_wdt.c
> linux-2.6.23-rc9-dirty/drivers/char/watchdog/sbc7240_wdt.c
> --- linux-2.6.23-rc9/drivers/char/watchdog/sbc7240_wdt.c	1970-01-01 10:00:00.000000000 +1000
> +++ linux-2.6.23-rc9-dirty/drivers/char/watchdog/sbc7240_wdt.c	2007-10-16 11:30:35.000000000 +1000


---
~Randy