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

[Markus Armbruster <armbru@redhat.com>]

Claudio Fontana <cfontana@suse.de> writes:

> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> ---
>  scripts/checkpatch.pl | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
>
> We could do something similar to MAINTAINERS for trace-events,
> ie warning about files added, moved, deleted if we don't see
> an update to a trace-events file.

I like the idea, but...

> To make it more solid it would be better to check the
> actual file contents for #include "trace.h" or "trace-root.h",
> but I guess this is not possible/practice from checkpatch?

... I'm also concerned about false positives.

> If we could only warn for files moved that actually include
> trace.h or trace-root.h, we could avoid a lot of false positives.

The existing MAINTAINERS check warns even when an existing pattern
covers the new file, e.g. when I create or rename a file scripts/qapi/*
or qapi/*.json.  The former is definitely a false positive, and mildly
annoying.  The latter, however, could be a true positive: even though
the new file is covered by the "QAPI Schema" section, *additional*
coverage by some other section may be called for, just like
qapi/machine.json is additionally covered by "Machine core".  So, even
if checkpatch.pl looked at more than just the patch, suppressing false
positives would involve guesswork.

The new trace-events check is simpler: it's *always* a false positive
when the file doesn't include trace.h or trace-root.h.

Complication: it could include via some header.  I figure that's rare
enough to be ignored.

Howver, checkpatch.pl checks *patches* by design[*].  It doesn't read
the patched files to get more context, or additional files.

Perhaps it's simply the wrong place both for the MAINTAINERS check and
the trace-events check.  We put the MAINTAINERS check there, because it
exists and developers run it.  "make check-source" would be another
option, except it doesn't exist.  CI would be yet another option, but we
should be careful about what to check only in CI.


[*] There's -f to check a source file, which checks "diff -u /dev/null
$filename".