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

[Markus Armbruster]

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.