[v5] checkpatch: add fix and improve warning msg for Non-standard signature
diff mbox series

Message ID 20201201112931.11192-1-yashsri421@gmail.com
State New, archived
Headers show
Series
  • [v5] checkpatch: add fix and improve warning msg for Non-standard signature
Related show

Commit Message

Aditya Srivastava Dec. 1, 2020, 11:29 a.m. UTC
Currently, checkpatch.pl warns for BAD_SIGN_OFF on non-standard signature
styles.

This warning occurs because of incorrect use of signature tags,
e.g. an evaluation on v4.13..v5.8 showed the use of following incorrect
signature tags, which may seem correct, but are not standard:

1) Requested-by (count: 48) => Suggested-by
Rationale: In an open-source project, there are no 'requests', just
'suggestions' to convince a maintainer to accept your patch

2) Co-authored-by (count: 43) => Co-developed-by
Rationale: Co-developed-by and Co-authored-by are synonyms

3) Analyzed-by (count: 22) / Analysed-by (count: 5) => Co-developed-by
Rationale: Analyzing is a part of Software Development, so
'Co-developed-by' is perfectly fine, even if contributor did not create
code

4) Improvements-by (count: 19) => Co-developed-by

5) Noticed-by (count: 11) => Reported-by

6) Inspired-by (count: 11) => Suggested-by

7) Verified-by (count: 8) => Tested-by
Rationale: Used by a single user. On reading mailing list, it seems
Tested-by might be a suitable alternative

8) Okay-ished-by (count: 8) => Acked-by
Rationale: Used by a single user. On reading mailing list, it seems
Acked-by must be suitable alternative

9) Acked-for-MFD-by (count: 6) => Acked-by

10) Reviewed-off-by (count: 5) => Reviewed-by

11) Proposed-by (count: 5) => Suggested-by
Rationale: On observing the mailing list, this tag is always used for a
maintainer. It seems that the changes might have been suggested by them
and the tag is used as acknowledgment for the same

12) Fixed-by (count: 3) => Co-developed-by
Rationale: Fixing bug is a part of Software Development, so
'Co-developed-by' is perfectly fine, even if contributor did not create
code

13) Pointed-out-by (count: 3) / Pointed-at-by (count: 2) => Suggested-by
Rationale: The tags are used for maintainers. It seems that the changes
might have been suggested by them and the tag is used as acknowledgment
for the same
E.g., Pointed-at-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

14) Suggestions-by (count: 3) => Suggested-by

Provide a fix by replacing the non-standard signature with its standard
equivalent.

Also, improve warning messages correspondingly, providing suitable
rationale to the user for the suggestion made.

Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
---
applies on next-20201120 and my last patch at Link: https://lore.kernel.org/linux-kernel-mentees/db1195235752685fc85fb52ecb1b1af3f35b5394.camel@perches.com/T/#u

changes in v2: replace commit specific example with brief evaluation

changes in v3: provide rationale to users for every signature tag suggestion;
modify commit message describing arrival to conclusion in a structured way

changes in v4: modify rationale for certain suggestions

changes in v5: remove the tag deletion suggestions, ie "Generated-by" and "Celebrated-by"; rebase on last accepted changes; modify commit message

 scripts/checkpatch.pl | 73 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 72 insertions(+), 1 deletion(-)

Comments

Joe Perches Dec. 1, 2020, 5:24 p.m. UTC | #1
On Tue, 2020-12-01 at 16:59 +0530, Aditya Srivastava wrote:
> Currently, checkpatch.pl warns for BAD_SIGN_OFF on non-standard signature
> styles.
>
> This warning occurs because of incorrect use of signature tags,
> e.g. an evaluation on v4.13..v5.8 showed the use of following incorrect
> signature tags, which may seem correct, but are not standard:

I'm not a fan of this patch.

There is already a "non-standard" signature warning for
all of these cases since 2012, predating the range of this
retrospective evaluation by over 5 years and yet these
existing commits have been accepted.

The value in actual standardization and effectively
requiring specific signature style tags is quite low.

Anyone that signed a thing a particular way should be free
to sign the thing as they choose.

Most of these warnings would also still be in the tree in
the future in new patches as running checkpatch without
it emitting a message of any type isn't a requirement nor
should checkpatch use actually be required workflow.
Lukas Bulwahn Dec. 1, 2020, 6:21 p.m. UTC | #2
On Tue, Dec 1, 2020 at 6:24 PM Joe Perches <joe@perches.com> wrote:
>
> On Tue, 2020-12-01 at 16:59 +0530, Aditya Srivastava wrote:
> > Currently, checkpatch.pl warns for BAD_SIGN_OFF on non-standard signature
> > styles.
> >
> > This warning occurs because of incorrect use of signature tags,
> > e.g. an evaluation on v4.13..v5.8 showed the use of following incorrect
> > signature tags, which may seem correct, but are not standard:
>
> I'm not a fan of this patch.
>
> There is already a "non-standard" signature warning for
> all of these cases since 2012, predating the range of this
> retrospective evaluation by over 5 years and yet these
> existing commits have been accepted.
>
> The value in actual standardization and effectively
> requiring specific signature style tags is quite low.
>
> Anyone that signed a thing a particular way should be free
> to sign the thing as they choose.
>
> Most of these warnings would also still be in the tree in
> the future in new patches as running checkpatch without
> it emitting a message of any type isn't a requirement nor
> should checkpatch use actually be required workflow.
>

Can we scale this fixing feature down to the very obvious synonyms
that simply do not add anything but confusion?

Such as for those four here:

Co-authored-by (count: 43) => Co-developed-by
Reviewed-off-by (count: 5) => Reviewed-by
Proposed-by (count: 5) => Suggested-by
Suggestions-by (count: 3) => Suggested-by

Then, we can probably also drop the rationale because it is pretty clear.

Of course, the impact might be really zero, given that it is unclear
if those authors did actually ever run checkpatch in the first place.

Joe, if you see no value in even such a minimal fix feature, let us
drop that idea and move on. There are enough other things to work on.

Lukas
Joe Perches Dec. 1, 2020, 6:39 p.m. UTC | #3
On Tue, 2020-12-01 at 19:21 +0100, Lukas Bulwahn wrote:
> On Tue, Dec 1, 2020 at 6:24 PM Joe Perches <joe@perches.com> wrote:
> > 
> > On Tue, 2020-12-01 at 16:59 +0530, Aditya Srivastava wrote:
> > > Currently, checkpatch.pl warns for BAD_SIGN_OFF on non-standard signature
> > > styles.
> > > 
> > > This warning occurs because of incorrect use of signature tags,
> > > e.g. an evaluation on v4.13..v5.8 showed the use of following incorrect
> > > signature tags, which may seem correct, but are not standard:
> > 
> > I'm not a fan of this patch.
> > 
> > There is already a "non-standard" signature warning for
> > all of these cases since 2012, predating the range of this
> > retrospective evaluation by over 5 years and yet these
> > existing commits have been accepted.
> > 
> > The value in actual standardization and effectively
> > requiring specific signature style tags is quite low.
> > 
> > Anyone that signed a thing a particular way should be free
> > to sign the thing as they choose.
> > 
> > Most of these warnings would also still be in the tree in
> > the future in new patches as running checkpatch without
> > it emitting a message of any type isn't a requirement nor
> > should checkpatch use actually be required workflow.
> > 
> 
> Can we scale this fixing feature down to the very obvious synonyms
> that simply do not add anything but confusion?
> 
> Such as for those four here:
> 
> Co-authored-by (count: 43) => Co-developed-by

I've never been a big fan of "Co-developed-by" as a signature tag,
but a "this should be that" here could be ok.

> Reviewed-off-by (count: 5) => Reviewed-by

I don't see value.  If no one notices a BAD_SIGN_OFF
for the Reviewed-off-by:, I doubt this would add anything.

> Proposed-by (count: 5) => Suggested-by
> Suggestions-by (count: 3) => Suggested-by

Suggestions-by is not suggested-by as these suggestions could
have been in response to an initial patch proposal and the
author could have incorporated those suggestions.

> Then, we can probably also drop the rationale because it is pretty clear.
> 
> Of course, the impact might be really zero, given that it is unclear
> if those authors did actually ever run checkpatch in the first place.
> 
> Joe, if you see no value in even such a minimal fix feature, let us
> drop that idea and move on. There are enough other things to work on.

Maybe only add the Co-authored-by: -> Co-developed-by: check.

But IMO: none of this is particularly useful.

Patch
diff mbox series

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 4a026926139f..d0c2f189272f 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -563,6 +563,72 @@  sub find_standard_signature {
 
 	return "";
 }
+our %standard_signature_fix = (
+	"Requested-by:" => {
+		suggestion => "Suggested-by:",
+		rationale => "In an open-source project, there are no 'requests', just 'suggestions' to convince a maintainer to accept your patch",
+	},
+	"Co-authored-by:" => {
+		suggestion => "Co-developed-by:",
+		rationale => "Co-developed-by is the standard signature tag to attribute multiple authors for a patch",
+	},
+	"Analyzed-by:" => {
+		suggestion => "Co-developed-by:",
+		rationale => "Analyzing is a part of Software Development, so 'Co-developed-by' is perfectly fine, even if contributor did not create code",
+	},
+	"Analysed-by:" => {
+		suggestion => "Co-developed-by:",
+		rationale => "Analysing is a part of Software Development, so 'Co-developed-by' is perfectly fine, even if contributor did not create code",
+	},
+	"Improvements-by:" => {
+		suggestion => "Co-developed-by:",
+		rationale => "Performing improvements are a part of Software Development, so 'Co-developed-by' is perfectly fine, even if contributor did not create code",
+	},
+	"Noticed-by:" => {
+		suggestion => "Reported-by:",
+		rationale => "Reported-by is the standard signature tag for acknowledging user who noticed or reported any bug(s)",
+	},
+	"Inspired-by:" => {
+		suggestion => "Suggested-by:",
+		rationale => "Suggested-by is the standard signature tag for acknowledging user for their suggestions",
+	},
+	"Verified-by:" => {
+		suggestion => "Tested-by:",
+		rationale => "Tested-by is the standard signature tag to attribute user for verifying/testing the patch",
+	},
+	"Okay-ished-by:" => {
+		suggestion => "Acked-by:",
+		rationale => "Acked-by is the standard signature tag for recording one's approval",
+	},
+	"Acked-for-MFD-by:" => {
+		suggestion => "Acked-by:",
+		rationale => "Acked-by is the standard signature tag for recording one's approval",
+	},
+	"Reviewed-off-by:" => {
+		suggestion => "Reviewed-by:",
+		rationale => "Reviewed-by is the standard signature tag to indicate that the patch has been reviewed",
+	},
+	"Proposed-by:" => {
+		suggestion => "Suggested-by:",
+		rationale => "Suggested-by is the standard signature tag for acknowledging user for their suggestions",
+	},
+	"Fixed-by:" => {
+		suggestion => "Co-developed-by:",
+		rationale => "Fixing bug is a part of Software Development, so 'Co-developed-by' is perfectly fine, even if contributor did not create code",
+	},
+	"Pointed-out-by:" => {
+		suggestion => "Suggested-by:",
+		rationale => "Pointing out certain changes is synonymous to suggesting changes, so Suggested-by seems perfectly fine",
+	},
+	"Pointed-at-by:" => {
+		suggestion => "Suggested-by:",
+		rationale => "Pointing at certain changes is synonymous to suggesting changes, so Suggested-by seems perfectly fine",
+	},
+	"Suggestions-by:" => {
+		suggestion => "Suggested-by:",
+		rationale => "Suggested-by is the standard signature tag for acknowledging user for their suggestions",
+	},
+);
 
 our @typeListMisordered = (
 	qr{char\s+(?:un)?signed},
@@ -2832,12 +2898,17 @@  sub process {
 
 			if ($sign_off !~ /$signature_tags/) {
 				my $suggested_signature = find_standard_signature($sign_off);
+				my $rationale = "";
+				if (exists($standard_signature_fix{$sign_off})) {
+					$suggested_signature = $standard_signature_fix{$sign_off}{'suggestion'};
+					$rationale = $standard_signature_fix{$sign_off}{'rationale'};
+				}
 				if ($suggested_signature eq "") {
 					WARN("BAD_SIGN_OFF",
 					     "Non-standard signature: $sign_off\n" . $herecurr);
 				} else {
 					if (WARN("BAD_SIGN_OFF",
-						 "Non-standard signature: '$sign_off' - perhaps '$suggested_signature'?\n" . $herecurr) &&
+						 "Non-standard signature: '$sign_off' - perhaps '$suggested_signature'? $rationale\n" . $herecurr) &&
 					    $fix) {
 						$fixed[$fixlinenr] =~ s/$sign_off/$suggested_signature/;
 					}