On 9/2/20 5:14 PM, Markus Armbruster wrote:
> Claudio Fontana <cfont...@suse.de> writes:
>
>> Signed-off-by: Claudio Fontana <cfont...@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
