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

[Fam Zheng]

On Mon, 04/16 16:09, Markus Armbruster wrote:
> Thomas Huth <address@hidden> writes:
> 
> > 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>
> 
> We should really keep upstream's S-o-b intact.  I'd keep the whole
> commit message intact:
> 
>     From 7fb819c27bf47041a13feed93f86a15bdb2c681f Mon Sep 17 00:00:00 2001
>     From: Joe Perches <address@hidden>
>     Date: Wed, 6 Aug 2014 16:10:59 -0700
>     Subject: [PATCH] checkpatch: emit a warning on file add/move/delete
> 
>     Whenever files are added, moved, or deleted, the MAINTAINERS file
>     patterns can be out of sync or outdated.
> 
>     To try to keep MAINTAINERS more up-to-date, add a one-time warning
>     whenever a patch does any of those.
> 
>     Signed-off-by: Joe Perches <address@hidden>
>     Acked-by: Andy Whitcroft <address@hidden>
>     Signed-off-by: Andrew Morton <address@hidden>
>     Signed-off-by: Linus Torvalds <address@hidden>
>     [Cherry picked from Linux commit 13f1937ef33950b1112049972249e6191b82e6c9]
>     Signed-off-by: Stefan Hajnoczi <address@hidden>
> 
> Created by running "git-format-patch -1 13f1937ef33" in the kernel,
> feeding that to git-am (patch doesn't apply), patch -p1 your patch,
> git-am --continue, git-commit --amend to add a backporting note and your
> S-o-b.
> 
> >> ---
> >> 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
> 
> Quoting that one:
> 
>     I think chances are high that it still pops up quite frequently with
>     false positives:
> 
>     1) The above regex only triggers for patches that contain a diffstat. If
>     you run the script on patches without diffstat, you always get the
>     warning as soon as you add, delete or move a file, even if you update
>     the MAINTAINERS file in the same patch.
> 
> "Doctor, it hurts when I create patches without a diffstat."
> 
>     2) I think it is quite common in patch series to first introduce new
>     files in the first patches, and then update MAINTAINERS only once at the
>     end.
> 
> That's an okay thing to do now.  But is it a valid reason to block
> tooling improvements that will help us stop the constant trickle of new
> files without a maintainer?  Having to update MAINTAINERS along the way
> isn't *that* much of a burden; we'll get used to it.
> 
>     3) The MAINTAINERS file often covers whole folders with wildcard
>     expressions. So if you add/delete/rename a file within such a folder,
>     you don't need to update MAINTAINERS thanks to the wildcard.
> 
> True.  Perhaps the kernel would appreciate a suitable checkpatch.pl
> improvement.
> 
>     I guess people might be annoyed if checkpatch.pl throws a warning in
>     these cases. So a "NOTE: ..." sounds more sane to me. But if you like,
>     we can also start with a WARNING first and only ease it if people start
>     to complain?
> 
> I'd stick to the upstream version.  But if it takes deviations to get
> this improvement accepted, I can live with them, as long as patchew
> still flags offenders.
> 

Patchew doesn't speak up unless there is an "error". Warnings and notes are not
complained about to keep good signal-to-noise ratio.

Fam