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 <joe@perches.com>.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
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
On Mon, Mar 12, 2018 at 2:18 PM, Stefan Hajnoczi <stefanha@redhat.com> 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 <joe@perches.com>.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
nice,
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> 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
On 12 March 2018 at 13:18, Stefan Hajnoczi <stefanha@redhat.com> 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 <joe@perches.com>.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
Unfortunately the patch doesn't magically cause maintainers
to appear for the new files :-)
thanks
-- PMM
Peter Maydell <peter.maydell@linaro.org> writes:
> On 12 March 2018 at 13:18, Stefan Hajnoczi <stefanha@redhat.com> 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 <joe@perches.com>.
>>
>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>
> 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 :)
On 16 April 2018 at 15:11, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> On 12 March 2018 at 13:18, Stefan Hajnoczi <stefanha@redhat.com> 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 <joe@perches.com>.
>>>
>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>
>> 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
Peter Maydell <peter.maydell@linaro.org> writes:
> On 16 April 2018 at 15:11, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> On 12 March 2018 at 13:18, Stefan Hajnoczi <stefanha@redhat.com> 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 <joe@perches.com>.
>>>>
>>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>>> ---
>>>
>>> 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?
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 <joe@perches.com>.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> 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
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 <joe@perches.com>.
> >
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> > 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
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 <joe@perches.com>.
>>>
>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>> 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
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 <joe@perches.com>.
> >>>
> >>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >>> ---
> >>> 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
Thomas Huth <thuth@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 <joe@perches.com>.
>>
>> Signed-off-by: Stefan Hajnoczi <stefanha@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 <joe@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 <joe@perches.com>
Acked-by: Andy Whitcroft <apw@canonical.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
[Cherry picked from Linux commit 13f1937ef33950b1112049972249e6191b82e6c9]
Signed-off-by: Stefan Hajnoczi <stefanha@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.
Reviewed-by: Markus Armbruster <armbru@redhat.com>
On Mon, 04/16 16:09, Markus Armbruster wrote:
> Thomas Huth <thuth@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 <joe@perches.com>.
> >>
> >> Signed-off-by: Stefan Hajnoczi <stefanha@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 <joe@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 <joe@perches.com>
> Acked-by: Andy Whitcroft <apw@canonical.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> [Cherry picked from Linux commit 13f1937ef33950b1112049972249e6191b82e6c9]
> Signed-off-by: Stefan Hajnoczi <stefanha@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
On 19.04.2018 04:06, Fam Zheng wrote:
> On Mon, 04/16 16:09, Markus Armbruster wrote:
>> Thomas Huth <thuth@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 <joe@perches.com>.
>>>>
>>>> Signed-off-by: Stefan Hajnoczi <stefanha@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 <joe@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 <joe@perches.com>
>> Acked-by: Andy Whitcroft <apw@canonical.com>
>> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
>> [Cherry picked from Linux commit 13f1937ef33950b1112049972249e6191b82e6c9]
>> Signed-off-by: Stefan Hajnoczi <stefanha@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
[...]
> 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
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20180312131806.23209-1-stefanha@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-devel@freelists.org
© 2016 - 2024 Red Hat, Inc.