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

[Claudio Fontana <cfontana@suse.de>]

On 9/2/20 5:14 PM, Markus Armbruster wrote:
> Claudio Fontana <cfontana@suse.de> writes:
> 
>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>> ---
>>  scripts/checkpatch.pl | 37 ++++++++++++++++++++++++++++++-------
>>  1 file changed, 30 insertions(+), 7 deletions(-)
>>
>> 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..37db212fc6 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);
>> +
> 
> Drop this blank line.
> 
>>  		} elsif ($line =~ /^\+\+\+\s+(\S+)/) {
>>  			$realfile = $1;
>>  			$realfile =~ s@^([^/]*)/@@ if (!$file);
>> @@ -1470,6 +1477,11 @@ sub process {
>>  			}
>>  
>>  			next;
>> +
> 
> And this one.
> 
>> +		} elsif ($line =~ /^---\s+(\S+)/) {
>> +			$fromfile = $1;
>> +			$fromfile =~ s@^([^/]*)/@@ if (!$file);
>> +			next;
>>  		}
>>  
>>  		$here .= "FILE: $realfile:$realline:" if ($realcnt != 0);
> 
> Aside: I don't understand why we need to match both the diff line and
> the --- line (and now the +++ line, too).
> 
>> @@ -1524,15 +1536,26 @@ sub process {
>>  		if ($line =~ /^\s*MAINTAINERS\s*\|/) {
>>  			$reported_maintainer_file = 1;
>>  		}
>> -
>> +# similar check for trace-events
>> +		if ($line =~ /^\s*trace-events\s*\|/) {
>> +			$reported_trace_events_file = 1;
>> +		}
> 
> These are meant to match in the diffstat (took me a stare to figure that
> out).
> 
> The existing one matches MAINTAINERS in the source root.  Good; that's
> where it is.
> 
> The new one matches trace-events in the source root.  Not so good; We
> use one trace-events per directory.
> 
> If I update trace-events in the source root, I won't be warned about
> other trace-events in need of updating (false negative), will I?
> 
> If I don't update trace-events in the source root, I will be warned
> regardless of what other trace-events I update (false positive), won't
> I?
> 
>>  # Check for added, moved or deleted files
>> -		if (!$reported_maintainer_file && !$in_commit_log &&
>> +		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))))) {
>> -			$reported_maintainer_file = 1;
>> -			WARN("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
>> +			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) {
>> +				if (`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);
>> +				}
>> +			}
>>  		}
>>  
>>  # Check for wrappage within a valid hunk of the file
> 
> Regarding Stefan's observations:
> 
> * Yes, the grep patterns need tightening.
> 
> * Yes, forking grep could be replaced by a simple function that slurps
>   in the file and searches it.  Could doesn't imply should, let alome
>   must.
> 
> As discussed in review of v1, I'm not sure checkpatch.pl is the best
> home for this kind of check.  I'm not going to reject a working patch
> because of that.
> 

Hi Marcus,

I will rethink this in the future, thanks for the useful feedback,
but I think this needs to be rethought, reworked and tested more.

Thanks!

Claudio