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

[Claudio Fontana]

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