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

[Thomas Huth <thuth@redhat.com>]

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