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

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

[Stefan Hajnoczi]

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

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

[Marc-André Lureau]

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

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

[Peter Maydell]

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

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

[Thomas Huth]

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

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

[no-reply]

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

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2345 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

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

[Thomas Huth]

[-- Attachment #1: Type: text/plain, Size: 2566 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3122 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

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

[Markus Armbruster]

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>

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

[Markus Armbruster]

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 :)

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

[Peter Maydell]

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

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

[Markus Armbruster]

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?

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

[Fam Zheng]

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

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

[Thomas Huth]

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