Re: [Qemu-devel] [PATCH] checkpatch: warn about missing MAINTAINERS file changes
On 19.04.2018 04:06, Fam Zheng wrote: > On Mon, 04/16 16:09, Markus Armbruster wrote: >> Thomas Huth 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 . Signed-off-by: Stefan Hajnoczi >> >> 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 >> 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 >> Acked-by: Andy Whitcroft >> Signed-off-by: Andrew Morton >> Signed-off-by: Linus Torvalds >> [Cherry picked from Linux commit >> 13f1937ef33950b1112049972249e6191b82e6c9] >> Signed-off-by: Stefan Hajnoczi >> >> 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 [...] > Patchew doesn't speak up unless there is an "error". Warnings and notes are > not > complained about to keep good signal-to-noise ratio. Ah, ok, didn't know that, that makes sense. Then I'm fine with the code above. But while you're at it, could you please also include the other patches that I posted last year, e.g. to warn on wrong utf-8 in the message? We've had mojibaked commit messages a couple of times already, and I hope that patch will help to reduce this a little bit... Thomas
Re: [Qemu-devel] [PATCH] checkpatch: warn about missing MAINTAINERS file changes
On Mon, 04/16 16:09, Markus Armbruster wrote: > Thomas Huth 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 . > >> > >> Signed-off-by: Stefan Hajnoczi > > 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 > 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 > Acked-by: Andy Whitcroft > Signed-off-by: Andrew Morton > Signed-off-by: Linus Torvalds > [Cherry picked from Linux commit 13f1937ef33950b1112049972249e6191b82e6c9] > Signed-off-by: Stefan Hajnoczi > > 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 pat
Re: [Qemu-devel] [PATCH] checkpatch: warn about missing MAINTAINERS file changes
Peter Maydell writes: > On 16 April 2018 at 15:11, Markus Armbruster wrote: >> Peter Maydell writes: >> >>> On 12 March 2018 at 13: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 . Signed-off-by: Stefan Hajnoczi --- >>> >>> Unfortunately the patch doesn't magically cause maintainers >>> to appear for the new files :-) >> >> Fortunately, the patch can get new files non-magically rejected unless a >> maintainers appears :) > > I think that "author of code lists themselves in MAINTAINERS > but then doesn't in practice do anything" is not really much > better (and arguably worse) than "code has no listed maintainer". Having our tooling flag new and moved files for a possible MAINTAINERS update need not mean adding J. Random Codeslinger to MAINTAINERS. It should make us stop and think. If the kernel guys can do that, why can't we?
Re: [Qemu-devel] [PATCH] checkpatch: warn about missing MAINTAINERS file changes
On 16 April 2018 at 15:11, Markus Armbruster wrote: > Peter Maydell writes: > >> On 12 March 2018 at 13: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 . >>> >>> Signed-off-by: Stefan Hajnoczi >>> --- >> >> Unfortunately the patch doesn't magically cause maintainers >> to appear for the new files :-) > > Fortunately, the patch can get new files non-magically rejected unless a > maintainers appears :) I think that "author of code lists themselves in MAINTAINERS but then doesn't in practice do anything" is not really much better (and arguably worse) than "code has no listed maintainer". thanks -- PMM
Re: [Qemu-devel] [PATCH] checkpatch: warn about missing MAINTAINERS file changes
Peter Maydell writes: > On 12 March 2018 at 13: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 . >> >> Signed-off-by: Stefan Hajnoczi >> --- > > Unfortunately the patch doesn't magically cause maintainers > to appear for the new files :-) Fortunately, the patch can get new files non-magically rejected unless a maintainers appears :)
Re: [Qemu-devel] [PATCH] checkpatch: warn about missing MAINTAINERS file changes
Thomas Huth 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 . >> >> Signed-off-by: Stefan Hajnoczi 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 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 Acked-by: Andy Whitcroft Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds [Cherry picked from Linux commit 13f1937ef33950b1112049972249e6191b82e6c9] Signed-off-by: Stefan Hajnoczi 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. Reviewed-by: Markus Armbruster
Re: [Qemu-devel] [PATCH] checkpatch: warn about missing MAINTAINERS file changes
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 . > >>> > >>> Signed-off-by: Stefan Hajnoczi > >>> --- > >>> 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
Re: [Qemu-devel] [PATCH] checkpatch: warn about missing MAINTAINERS file changes
Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: 20180312131806.23209-1-stefa...@redhat.com Subject: [Qemu-devel] [PATCH] checkpatch: warn about missing MAINTAINERS file changes === TEST SCRIPT BEGIN === #!/bin/bash BASE=base n=1 total=$(git log --oneline $BASE.. | wc -l) failed=0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..." if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then failed=1 echo fi n=$((n+1)) done exit $failed === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' 89d665069d checkpatch: warn about missing MAINTAINERS file changes === OUTPUT BEGIN === Checking PATCH 1/1: checkpatch: warn about missing MAINTAINERS file changes... WARNING: line over 80 characters #47: FILE: scripts/checkpatch.pl:1393: +($line =~ /\{\s*([\w\/\.\-]*)\s*\=\>\s*([\w\/\.\-]*)\s*\}/ && ERROR: line over 90 characters #51: FILE: scripts/checkpatch.pl:1397: + WARN("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . total: 1 errors, 1 warnings, 31 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-de...@freelists.org
Re: [Qemu-devel] [PATCH] checkpatch: warn about missing MAINTAINERS file changes
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 . >>> >>> Signed-off-by: Stefan Hajnoczi >>> --- >>> 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. Thomas signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH] checkpatch: warn about missing MAINTAINERS file changes
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 . > > > > Signed-off-by: Stefan Hajnoczi > > --- > > 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? Stefan signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH] checkpatch: warn about missing MAINTAINERS file changes
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 . > > Signed-off-by: Stefan Hajnoczi > --- > 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 Thanks, Thomas
Re: [Qemu-devel] [PATCH] checkpatch: warn about missing MAINTAINERS file changes
On Mon, Mar 12, 2018 at 2:18 PM, 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 . > > Signed-off-by: Stefan Hajnoczi nice, Reviewed-by: Marc-André Lureau > --- > 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); > + } > + > # Check for wrappage within a valid hunk of the file > if ($realcnt != 0 && $line !~ m{^(?:\+|-| |\\ No newline|$)}) > { > ERROR("patch seems to be corrupt (line wrapped?)\n" . > -- > 2.14.3 > > -- Marc-André Lureau
Re: [Qemu-devel] [PATCH] checkpatch: warn about missing MAINTAINERS file changes
On 12 March 2018 at 13: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 . > > Signed-off-by: Stefan Hajnoczi > --- Unfortunately the patch doesn't magically cause maintainers to appear for the new files :-) thanks -- PMM
[Qemu-devel] [PATCH] checkpatch: warn about missing MAINTAINERS file changes
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 . Signed-off-by: Stefan Hajnoczi --- 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); + } + # Check for wrappage within a valid hunk of the file if ($realcnt != 0 && $line !~ m{^(?:\+|-| |\\ No newline|$)}) { ERROR("patch seems to be corrupt (line wrapped?)\n" . -- 2.14.3