Re: [Qemu-devel] [PATCH] checkpatch: warn about missing MAINTAINERS file changes

[Stefan Hajnoczi]

On Tue, Mar 13, 2018 at 11:49:57AM +0100, Thomas Huth wrote:
> On 13.03.2018 11:37, Stefan Hajnoczi wrote:
> > On Mon, Mar 12, 2018 at 02:46:20PM +0100, Thomas Huth wrote:
> >> On 12.03.2018 14:18, Stefan Hajnoczi wrote:
> >>> Warn if files are added/renamed/deleted without MAINTAINERS file
> >>> changes.  This has helped me in Linux and we could benefit from this
> >>> check in QEMU.
> >>>
> >>> This patch is a manual cherry-pick of Linux commit
> >>> 13f1937ef33950b1112049972249e6191b82e6c9 ("checkpatch: emit a warning on
> >>> file add/move/delete") by Joe Perches <address@hidden>.
> >>>
> >>> Signed-off-by: Stefan Hajnoczi <address@hidden>
> >>> ---
> >>> Note the 80-char lines are from upstream code.  Keep them as-is.
> >>>
> >>>  scripts/checkpatch.pl | 19 +++++++++++++++++++
> >>>  1 file changed, 19 insertions(+)
> >>>
> >>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> >>> index d1fe79bcc4..d0d8f63d48 100755
> >>> --- a/scripts/checkpatch.pl
> >>> +++ b/scripts/checkpatch.pl
> >>> @@ -1177,6 +1177,7 @@ sub process {
> >>>   our $clean = 1;
> >>>   my $signoff = 0;
> >>>   my $is_patch = 0;
> >>> + my $reported_maintainer_file = 0;
> >>>  
> >>>   our @report = ();
> >>>   our $cnt_lines = 0;
> >>> @@ -1379,6 +1380,24 @@ sub process {
> >>>                   }
> >>>           }
> >>>  
> >>> +# Check if MAINTAINERS is being updated.  If so, there's probably no 
> >>> need to
> >>> +# emit the "does MAINTAINERS need updating?" message on file 
> >>> add/move/delete
> >>> +         if ($line =~ /^\s*MAINTAINERS\s*\|/) {
> >>> +                 $reported_maintainer_file = 1;
> >>> +         }
> >>> +
> >>> +# Check for added, moved or deleted files
> >>> +         if (!$reported_maintainer_file &&
> >>> +             ($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))))) {
> >>> +                 $is_patch = 1;
> >>> +                 $reported_maintainer_file = 1;
> >>> +                 WARN("added, moved or deleted file(s), does MAINTAINERS 
> >>> need updating?\n" .
> >>> +                         $herecurr);
> >>
> >> Could you please turn this into a notification instead of a warning? For
> >> rationale, please see the discussion of this patch last year:
> >>
> >> http://lists.gnu.org/archive/html/qemu-devel/2017-01/msg05753.html
> > 
> > It's a warning, not an error, so this already means "don't treat it as
> > fatal".
> > 
> > Why is a warning a bad user experience but a notification would be fine?
> 
> Since this will likely cause a lot of false positives. I think people
> will then rather be annoyed if checkpatch.pl nags them with a warning in
> these cases.

This approach works fine for Linux, I don't think it will be a problem
for QEMU.

The idea of zero false positives is nice though.  Do you have time to
implement a real MAINTAINERS checker that avoids false positives (i.e.
it applies the MAINTAINERS hunk in the patch, if present, onto the
current MAINTAINERS file and then looks up every new file or rename)?

Stefan

signature.asc
Description: PGP signature