linux-kernel - Re: [PATCH v3 68/77] ncr5380: Fix whitespace issues using regexp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.LNX.2.00.1512231454350.32341@nippy.intranet>
Date:	Wed, 23 Dec 2015 15:14:28 +1100 (AEDT)
From:	Finn Thain <fthain@...egraphics.com.au>
To:	Joe Perches <joe@...ches.com>
cc:	"James E.J. Bottomley" <JBottomley@...n.com>,
	Michael Schmitz <schmitzmic@...il.com>,
	linux-m68k@...r.kernel.org, linux-scsi@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	"Martin K. Petersen" <martin.petersen@...cle.com>
Subject: Re: [PATCH v3 68/77] ncr5380: Fix whitespace issues using regexp


On Tue, 22 Dec 2015, Joe Perches wrote:

> On Wed, 2015-12-23 at 13:03 +1100, Finn Thain wrote:
> > On Tue, 22 Dec 2015, Joe Perches wrote:
> > 
> > > On Wed, 2015-12-23 at 11:56 +1100, Finn Thain wrote:
> > > > On Tue, 22 Dec 2015, Joe Perches wrote:
> > > > 
> > > > > On Tue, 2015-12-22 at 12:18 +1100, Finn Thain wrote:
> > > > > > This patch is just the result of two substitutions. The first 
> > > > > > removes any tabs and spaces at the end of the line. The second 
> > > > > > replaces runs of tabs and spaces at the beginning of comment lines 
> > > > > > with a single space.
> > > > > 
> > > > > I think the second of these isn't done well.
> > > > 
> > > > The aim of this patch is not to fix code style, but to make it 
> > > > possible to compare these two files so that the fork can be repaired. 
> > > > Regexp is very helpful in creating uniformity (and a minimal diff).
> > > > 
> > > > If this was a coding style issue, we would be discussing the use of 
> > > > kernel-doc format for the affected comments, not whitespace.
> > > > 
> > > > > Many of these comments post reformatting are much worse to read 
> > > > > because of lost alignment.
> > > > 
> > > > You exaggerate a very trivial point.
> > > 
> > > 
> > > 
> > > I prefer that all patches be improvements.
> > > 
> > 
> > Agreed. But the example you cited is an improvement, in that it creates 
> > consistency.
> 
> I think "consistency" isn't a useful argument.
> The kernel code doesn't care about any other
> external code bases.

I prefer that the drivers I maintain be self-consistent.

> 
> > Like you, I prefer to see formal parameters aligned when wrapped. But this 
> > isn't a formal parameter list, it is a comment, and no comment should 
> > duplicate code.
> > 
> > Can you suggest a better regexp? Since this is patch 68 in the series, 
> > there is a good chance that it will need to be regenerated.
> 
> I suggest you do 2 patches here.  One that removes
> unnecessary trailing spaces

Those are resolved by this patch.

> and converts multiple leading spaces to tabs where appropriate

As I said, trivial cleanups are better done after the fork is resolved, to 
avoid churn. To assist with resolving the fork, this patch addresses 
inconsistencies.

> and a
> second patch that fixes whatever odd indentation
> that does exist after comment leading *.

Those are resolved by this patch.

> I think
> there aren't many instances of those and I think
> those should be done by hand rather than regex.

I don't know why a regexp wouldn't work and I don't know what you have in 
mind when you say "[fix] odd indentation". Is there some kind of style 
guide applicable here, which this patch violates?

Upon re-reading this patch, I did find a table where I think the regexp is 
detrimental.

@@ -2096,7 +2096,7 @@ static void NCR5380_information_transfer
 					 * Byte
 					 * 0		EXTENDED_MESSAGE == 1
 					 * 1		length (includes one byte for code, doesn't
-					 *		include first two bytes)
+					 * include first two bytes)
 					 * 2		code
 					 * 3..length+1	arguments
 					 *
This table is interesting. Even though the author took the trouble to
duplicate a portion of the SCSI spec in the source, they were still able 
to stuff it up. See patch 44/77 in this series, "ncr5380: Fix off-by-one 
bug in extended_msg[] bounds check".

So how about I remove this table in patch 67, along with the other dud 
comments, and then regenerate this patch. That way, perhaps we can all 
agree that the regexp is not actually detrimental?

--