Message-ID: <1433278159.4861.99.camel@perches.com>
Subject: Re: [PATCH] checkpatch: Make the output better readable
From: Joe Perches <joe@perches.com>
To: Petr Mladek <pmladek@suse.cz>
Cc: Andy Whitcroft <apw@canonical.com>, linux-kernel@vger.kernel.org
Date: Tue, 02 Jun 2015 13:49:19 -0700
In-Reply-To: <20150602104434.GL3135@pathway.suse.cz>
References: <1433168737-17009-1-git-send-email-pmladek@suse.cz>
	 <1433173824.4861.8.camel@perches.com>
	 <20150602091340.GJ3135@pathway.suse.cz>
	 <1433238723.4861.76.camel@perches.com>
	 <20150602104434.GL3135@pathway.suse.cz>
Content-Type: text/plain; charset="ISO-8859-1"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 6506
Lines: 217

On Tue, 2015-06-02 at 12:44 +0200, Petr Mladek wrote:
> On Tue 2015-06-02 02:52:03, Joe Perches wrote:
> > On Tue, 2015-06-02 at 11:13 +0200, Petr Mladek wrote:
> > > On Mon 2015-06-01 08:50:24, Joe Perches wrote:
> > > > On Mon, 2015-06-01 at 16:25 +0200, Petr Mladek wrote:
> > > > > I always have troubles to parse checkpatch.pl output when I check
> > > > > the whole patchset. It is hard to say which messages belongs to
> > > > > what patch.
> > > > > 
> > > > > This patch does few small changes to make the output look better
> > > > > for me:
> > > > > 
> > > > >     + delimit each patch from each other with dashes and empty line
> > > > >     + remove empty line after the summary
> > > > 
> > > > I've no objection about this, but don't much care either.
> > > > 
> > > > >     + print message about false positives only once
> > > > 
> > > > This bit seems sensible, thanks.
> > > > 
> > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > > []
> > > > > @@ -720,8 +720,14 @@ my @fixed_deleted = ();
> > > > >  my $fixlinenr = -1;
> > > > >  
> > > > >  my $vname;
> > > > > +my $filenum = 0;
> > > > >  for my $filename (@ARGV) {
> > > > >  	my $FILE;
> > > > > +
> > > > > +	if ($filenum++ && $quiet == 0) {
> > > > > +		print "--------------------------------------------------------------------------------\n";
> > > > 
> > > > Perhaps more perlish would be print '-' x 81 . '\n\n';
> > > > Dunno why you chose 81 though, it seems an unusual number.
> > > 
> > > Are you sure, please? I have just counted it again and I see 80
> > > dashes. Is it possible that you counted the initial quotation
> > > mark '"'?
> > 
> > My mistake, I neglected to account for the cr in echo|wc
> > 
> > > Well, I do not mind about the number of dashes. Feel free to update
> > > it in case you merge it.
> > 
> > I don't actually merge stuff, I can forward it to
> > Andrew Morton though,
> 
> That would be nice. Thanks in advance.
> 
> > but perhaps it'd be better to
> > check $#ARGV > 1 and emit something like
> > 	"$filename is being processed\n"
> > so that there is a delimiter before and after each file
> 
> I personally do not like this idea much. It would create another
> long line and kind of hide the warnings and errors. IMHO, the dashes
> are better and enough. But I am not UI guy.
> 
> But feel free to improve it as you like.
> 
> > Another option for you is to add --emacs on the command line.
> > That prefixes patch filename & location before each message.
> 
> Thanks for the hint. I was not aware of it. Well, it still looks
> messy without my patch.

Maybe this:

If there are multiple patches/files on the command line,
use a prefix before the patch/file message output like:
	--------------
	patch/filename
	--------------
to make the identifying which messages go with which
file/patch a bit easier to parse.

Move the perl version and false positive messages after
all the files have been scanned so that they are emitted
only once.

Standardize the NOTE: <...> form to always emit a blank
line before the NOTE and always use print << "EOM" style.
---
 scripts/checkpatch.pl | 62 ++++++++++++++++++++++++++++++++-------------------
 1 file changed, 39 insertions(+), 23 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index c8032a0..eaa76bd 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -197,11 +197,11 @@ sub hash_show_words {
 	my ($hashRef, $prefix) = @_;
 
 	if ($quiet == 0 && keys %$hashRef) {
-		print "NOTE: $prefix message types:";
+		print "\nNOTE: $prefix message types:";
 		foreach my $word (sort keys %$hashRef) {
 			print " $word";
 		}
-		print "\n\n";
+		print "\n";
 	}
 }
 
@@ -741,6 +741,13 @@ for my $filename (@ARGV) {
 		push(@rawlines, $_);
 	}
 	close($FILE);
+
+	if ($#ARGV > 0 && $quiet == 0) {
+		print '-' x length($vname) . "\n";
+		print "$vname\n";
+		print '-' x length($vname) . "\n";
+	}
+
 	if (!process($filename)) {
 		$exit = 1;
 	}
@@ -755,6 +762,23 @@ for my $filename (@ARGV) {
 	build_types();
 }
 
+if (!$quiet) {
+	if ($^V lt 5.10.0) {
+		print << "EOM"
+
+NOTE: perl $^V is not modern enough to detect all possible issues.
+      An upgrade to at least perl v5.10.0 is suggested.
+EOM
+	}
+	if ($exit) {
+		print << "EOM"
+
+NOTE: If any of the errors are false positives, please report
+      them to the maintainer, see CHECKPATCH in MAINTAINERS.
+EOM
+	}
+}
+
 exit($exit);
 
 sub top_of_kernel_tree {
@@ -5578,22 +5602,18 @@ sub process {
 		print "total: $cnt_error errors, $cnt_warn warnings, " .
 			(($check)? "$cnt_chk checks, " : "") .
 			"$cnt_lines lines checked\n";
-		print "\n" if ($quiet == 0);
 	}
 
 	if ($quiet == 0) {
-
-		if ($^V lt 5.10.0) {
-			print("NOTE: perl $^V is not modern enough to detect all possible issues.\n");
-			print("An upgrade to at least perl v5.10.0 is suggested.\n\n");
-		}
-
 		# If there were whitespace errors which cleanpatch can fix
 		# then suggest that.
 		if ($rpt_cleaners) {
-			print "NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch or\n";
-			print "      scripts/cleanfile\n\n";
 			$rpt_cleaners = 0;
+			print << "EOM"
+
+NOTE: Whitespace errors detected.
+      You may wish to use scripts/cleanpatch or scripts/cleanfile
+EOM
 		}
 	}
 
@@ -5627,6 +5647,7 @@ sub process {
 
 		if (!$quiet) {
 			print << "EOM";
+
 Wrote EXPERIMENTAL --fix correction(s) to '$newfile'
 
 Do _NOT_ trust the results written to this file.
@@ -5634,22 +5655,17 @@ Do _NOT_ submit these changes without inspecting them for correctness.
 
 This EXPERIMENTAL file is simply a convenience to help rewrite patches.
 No warranties, expressed or implied...
-
 EOM
 		}
 	}
 
-	if ($clean == 1 && $quiet == 0) {
-		print "$vname has no obvious style problems and is ready for submission.\n"
-	}
-	if ($clean == 0 && $quiet == 0) {
-		print << "EOM";
-$vname has style problems, please review.
-
-If any of these errors are false positives, please report
-them to the maintainer, see CHECKPATCH in MAINTAINERS.
-EOM
+	if ($quiet == 0) {
+		print "\n";
+		if ($clean == 1) {
+			print "$vname has no obvious style problems and is ready for submission.\n";
+		} else {
+			print "$vname has style problems, please review.\n";
+		}
 	}
-
 	return $clean;
 }


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/