[v3] checkpatch: detect missing changes to trace-events

[RFC v3] checkpatch: detect missing changes to trace-events

Posted by Claudio Fontana 3 years, 8 months ago

Signed-off-by: Claudio Fontana <cfontana@suse.de>
---
 scripts/checkpatch.pl | 48 ++++++++++++++++++++++++++++++++++++------------
 1 file changed, 36 insertions(+), 12 deletions(-)

v2 -> v3 :

* move the check for missing changes to MAINTAINERS and trace-events
  later on, as otherwise the check for in_commit_log will not be done
  on an up to date value, causing fromfile and realfile to not be set,
  and matching unwanted strings inside the commit log.

* Ensure that at least one file name is passed to grep.


----

v1 -> v2 :

* track the "from" file in addition to the "to" file,
  and grep into both (if they exist), looking for trace.h, trace-root.h

  If files are reachable and readable, emit a warning if there is no
  update to trace-events.


diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index bd3faa154c..f63013bc35 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1300,6 +1300,7 @@ sub process {
 	my $in_header_lines = $file ? 0 : 1;
 	my $in_commit_log = 0;		#Scanning lines before patch
 	my $reported_maintainer_file = 0;
+	my $reported_trace_events_file = 0;
 	my $non_utf8_charset = 0;
 
 	our @report = ();
@@ -1309,6 +1310,7 @@ sub process {
 	our $cnt_chk = 0;
 
 	# Trace the real file/line as we go.
+	my $fromfile = '';
 	my $realfile = '';
 	my $realline = 0;
 	my $realcnt = 0;
@@ -1454,10 +1456,15 @@ sub process {
 		$here = "#$realline: " if ($file);
 
 		# extract the filename as it passes
-		if ($line =~ /^diff --git.*?(\S+)$/) {
-			$realfile = $1;
-			$realfile =~ s@^([^/]*)/@@ if (!$file);
+		if ($line =~ /^diff --git.*?(\S+).*?(\S+)$/) {
+			$fromfile = $1;
+			$realfile = $2;
+			if (!$file) {
+				$fromfile =~ s@^([^/]*)/@@ ;
+				$realfile =~ s@^([^/]*)/@@ ;
+			}
 	                checkfilename($realfile, \$acpi_testexpected, \$acpi_nontestexpected);
+
 		} elsif ($line =~ /^\+\+\+\s+(\S+)/) {
 			$realfile = $1;
 			$realfile =~ s@^([^/]*)/@@ if (!$file);
@@ -1470,6 +1477,11 @@ sub process {
 			}
 
 			next;
+
+		} elsif ($line =~ /^---\s+(\S+)/) {
+			$fromfile = $1;
+			$fromfile =~ s@^([^/]*)/@@ if (!$file);
+			next;
 		}
 
 		$here .= "FILE: $realfile:$realline:" if ($realcnt != 0);
@@ -1524,15 +1536,9 @@ sub process {
 		if ($line =~ /^\s*MAINTAINERS\s*\|/) {
 			$reported_maintainer_file = 1;
 		}
-
-# Check for added, moved or deleted files
-		if (!$reported_maintainer_file && !$in_commit_log &&
-		    ($line =~ /^(?:new|deleted) file mode\s*\d+\s*$/ ||
-		     $line =~ /^rename (?:from|to) [\w\/\.\-]+\s*$/ ||
-		     ($line =~ /\{\s*([\w\/\.\-]*)\s*\=\>\s*([\w\/\.\-]*)\s*\}/ &&
-		      (defined($1) || defined($2))))) {
-			$reported_maintainer_file = 1;
-			WARN("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
+# similar check for trace-events
+		if ($line =~ /^\s*trace-events\s*\|/) {
+			$reported_trace_events_file = 1;
 		}
 
 # Check for wrappage within a valid hunk of the file
@@ -1605,6 +1611,24 @@ sub process {
 			$rpt_cleaners = 1;
 		}
 
+# Check for added, moved or deleted files
+		if (!$in_commit_log &&
+		    ($line =~ /^(?:new|deleted) file mode\s*\d+\s*$/ ||
+		     $line =~ /^rename (?:from|to) [\w\/\.\-]+\s*$/ ||
+		     ($line =~ /\{\s*([\w\/\.\-]*)\s*\=\>\s*([\w\/\.\-]*)\s*\}/ &&
+		      (defined($1) || defined($2))))) {
+			if (!$reported_maintainer_file) {
+				$reported_maintainer_file = 1;
+				WARN("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
+			}
+			if (!$reported_trace_events_file &&
+			    ($fromfile ne '' || $realfile ne '') &&
+			    (`grep -F -s -e trace.h -e trace-root.h ${fromfile} ${realfile}` ne '')) {
+				$reported_trace_events_file = 1;
+				WARN("added, moved or deleted file(s), does trace-events need updating?\n" . $herecurr);
+			}
+		}
+
 # checks for trace-events files
 		if ($realfile =~ /trace-events$/ && $line =~ /^\+/) {
 			if ($rawline =~ /%[-+ 0]*#/) {
-- 
2.16.4

Re: [RFC v3] checkpatch: detect missing changes to trace-events

Posted by Stefan Hajnoczi 3 years, 7 months ago

On Tue, Aug 11, 2020 at 10:11:58AM +0200, Claudio Fontana wrote:
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> ---
>  scripts/checkpatch.pl | 48 ++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 36 insertions(+), 12 deletions(-)
> 
> v2 -> v3 :
> 
> * move the check for missing changes to MAINTAINERS and trace-events
>   later on, as otherwise the check for in_commit_log will not be done
>   on an up to date value, causing fromfile and realfile to not be set,
>   and matching unwanted strings inside the commit log.
> 
> * Ensure that at least one file name is passed to grep.
> 
> 
> ----
> 
> v1 -> v2 :
> 
> * track the "from" file in addition to the "to" file,
>   and grep into both (if they exist), looking for trace.h, trace-root.h
> 
>   If files are reachable and readable, emit a warning if there is no
>   update to trace-events.

This patch still has an RFC tag. Is it ready to be merged?

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [RFC v3] checkpatch: detect missing changes to trace-events

Posted by Claudio Fontana 3 years, 7 months ago

On 9/14/20 3:44 PM, Stefan Hajnoczi wrote:
> On Tue, Aug 11, 2020 at 10:11:58AM +0200, Claudio Fontana wrote:
>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>> ---
>>  scripts/checkpatch.pl | 48 ++++++++++++++++++++++++++++++++++++------------
>>  1 file changed, 36 insertions(+), 12 deletions(-)
>>
>> v2 -> v3 :
>>
>> * move the check for missing changes to MAINTAINERS and trace-events
>>   later on, as otherwise the check for in_commit_log will not be done
>>   on an up to date value, causing fromfile and realfile to not be set,
>>   and matching unwanted strings inside the commit log.
>>
>> * Ensure that at least one file name is passed to grep.
>>
>>
>> ----
>>
>> v1 -> v2 :
>>
>> * track the "from" file in addition to the "to" file,
>>   and grep into both (if they exist), looking for trace.h, trace-root.h
>>
>>   If files are reachable and readable, emit a warning if there is no
>>   update to trace-events.
> 
> This patch still has an RFC tag. Is it ready to be merged?
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> 

Hi Stefan, I think it's actually not, I would like to have more testing on it, checking all false positive/false negative cases.
I found some issues with it, with false negatives, that I still do not understand.

If anybody wants to rework this taking this as starting point, that's good, otherwise I will get back to it when I can..

I am not sure about the "move the check" part in v2 -> v3, and if anybody has more input and can help to understand this aspect, welcome to collaborate on this.

Ciao,

CLaudio