Re: Re: Re: Re: checkpatch: support deprecated terms checking

[SeongJae Park <sj38.park@gmail.com>]

On Sun, 26 Jul 2020 00:29:05 -0700 Joe Perches <joe@perches.com> wrote:

> On Sun, 2020-07-26 at 09:18 +0200, SeongJae Park wrote:
> > On Sat, 25 Jul 2020 21:27:07 -0700 Joe Perches <joe@perches.com> wrote:
> > 
> > > On Sun, 2020-07-26 at 01:35 +0200, SeongJae Park wrote:
> > > > On Sat, 25 Jul 2020 10:29:23 -0700 Joe Perches <joe@perches.com> wrote:
> > > > 
> > > > > On Sat, 2020-07-25 at 15:02 +0200, Michał Mirosław wrote:
> > > > > > Hello,
> > > > > > 
> > > > > > I see that this patch went into next and is already inciting people to
> > > > > > do wrong things [1]. Can you please fix it to require '--subjective'
> > > > > > switch or otherwise mark it clearly as suggestion-only?
> > > > > > 
> > > > > > The coding-style as in Linus' master says about *NEW* uses of the words
> > > > > > listed (those introductions I expect to be actually rare) and not about
> > > > > > existing use in the code or industry. Making a noise about all uses
> > > > > > found surely will generate a lot more irrelevant patches.
> > > > > > 
> > > > > > [1] https://www.spinics.net/lists/linux-tegra/msg51849.html
> > > > > 
> > > > > And if not reverted, perhaps do not check existing files
> > > > > at all but only check patches and change the message to
> > > > > show only suggestions not from a specification.
> > > > 
> > > > Agreed for this case.  However, excluding existing file check doesn't fully
> > > > avoid this problem.  Also, more terms having different deprecation rules might
> > > > be added in future.  How about allowing file check but show reference in the
> > > > suggestion message as below?
> > > 
> > > The general problem is that drivers/staging, net/ and drivers/net
> > > all have --strict on by default.
> > > 
> > > Emitting these deprecated terms messages with -f --file uses for
> > > files in those directories isn't a great idea.
> > 
> > Thank you for kindly explaining your concenrs in detail.  However, I think it's
> > ok to do this check even without '--strict' for files if we explicitly says
> > it's suggestion only, as Michal said.  My patch does so.
> > 
> > > > diff --git a/scripts/deprecated_terms.txt b/scripts/deprecated_terms.txt
> > > []
> > > > @@ -3,8 +3,10 @@
> > > >  # The format of each line is:
> > > >  # deprecated||suggested
> > > >  #
> > > > +# If special rules are applied on the terms, please comment those.
> > > 
> > > Disagree.  Comments about these existing uses aren't helpful.
> > 
> > Sorry, I don't understand your point here.  Why do you think it's not helpful?
> > If 'checkpatch' finds the deprecated terms, it will ask people to read this
> > file, which explains special rules for each of the deprecations if exists.  The
> > rule is, in the case of 'slave', 'applies to new uses only'.  Therefore, people
> > could stop sending the noisy unnecessary patches to the maintainers.
> 
> Because it will describe this for _every_ instance
> of any deprecated word in the file.

Thank you for kindly explaining your concern.  I personally thought the verbose
warning is not a real problem.  Anyway, how about below patch, then?  It will
show only one warning or check for each of the terms.

================================= >8 ==========================================

From 6c606c62ea25933db8bb0afec083b5b4b8b3f11f Mon Sep 17 00:00:00 2001
From: SeongJae Park <sjpark@amazon.de>
Date: Sun, 26 Jul 2020 01:14:48 +0200
Subject: [PATCH] scripts/deprecatd_terms: provide references

Deprecation of terms could have special rules.  For example, 'slave' is
ok for existing usages.  Same to 'master', but it's also ok unless it's
used with 'slave'.  This commit provides the references for such rules.

Also, because the report became more verbose a little, this commit makes
the report to be made for only one instance of each deprecated term.

Signed-off-by: SeongJae Park <sjpark@amazon.de>
---
 scripts/checkpatch.pl        | 6 +++++-
 scripts/deprecated_terms.txt | 6 ++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e9fde28eb0de..227e088bfe56 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -721,6 +721,7 @@ sub read_word_corrections {
 my %deprecated_terms_fix;
 read_word_corrections($deprecated_terms_file, \%deprecated_terms_fix);
 my $deprecated_terms = join("|", sort keys %deprecated_terms_fix) if keys %deprecated_terms_fix;
+my %deprecated_terms_reported = map { $_ => 1 }
 
 # Load common spelling mistakes and build regular expression list.
 my $misspellings;
@@ -2975,13 +2976,16 @@ sub process {
 		    ($in_commit_log || $line =~ /^(?:\+|Subject:)/i)) {
 			while ($rawline =~ /(?:^|[^a-z@])($deprecated_terms)(?:\b|$|[^a-z@])/gi) {
 				my $deprecated_term = $1;
+				last if (exists($deprecated_terms_reported{$deprecated_term}));
+				$deprecated_terms_reported{$deprecated_term} = 1;
+
 				my $suggested = $deprecated_terms_fix{lc($deprecated_term)};
 				$suggested = ucfirst($suggested) if ($deprecated_term=~ /^[A-Z]/);
 				$suggested = uc($suggested) if ($deprecated_term =~ /^[A-Z]+$/);
 				my $msg_level = \&WARN;
 				$msg_level = \&CHK if ($file);
 				if (&{$msg_level}("DEPRECATED_TERM",
-						  "Use of '$deprecated_term' is deprecated, please '$suggested', instead.\n" . $herecurr) &&
+						  "Use of '$deprecated_term' is controversial - if not required by specification, perhaps '$suggested' instead.  See: scripts/deprecated_terms.txt\n" . $herecurr) &&
 				    $fix) {
 					$fixed[$fixlinenr] =~ s/(^|[^A-Za-z@])($deprecated_term)($|[^A-Za-z@])/$1$suggested$3/;
 				}
diff --git a/scripts/deprecated_terms.txt b/scripts/deprecated_terms.txt
index 1be27a24187b..d92b9c896fce 100644
--- a/scripts/deprecated_terms.txt
+++ b/scripts/deprecated_terms.txt
@@ -3,8 +3,10 @@
 # The format of each line is:
 # deprecated||suggested
 #
+# If special rules are applied on the terms, please comment those.
+#
+# Refer to "4) Naming" section of Documentation/process/coding-style.rst for
+# below three terms.
 blacklist||(denylist|blocklist)
-# For other alternatives of 'slave', Please refer to
-# Documentation/process/coding-style.rst
 slave||(secondary|target|...)
 whitelist||(allowlist|passlist)
-- 
2.17.1