On Mon, 04/16 16:09, Markus Armbruster wrote:
> Thomas Huth <th...@redhat.com> 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 <j...@perches.com>.
> >>
> >> Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com>
>
> 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 <j...@perches.com>
> 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 <j...@perches.com>
> Acked-by: Andy Whitcroft <a...@canonical.com>
> Signed-off-by: Andrew Morton <a...@linux-foundation.org>
> Signed-off-by: Linus Torvalds <torva...@linux-foundation.org>
> [Cherry picked from Linux commit 13f1937ef33950b1112049972249e6191b82e6c9]
> Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com>
>
> 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
