Skip to contentSkip to conversation

checkpatch: add fix option for MISSING_SIGN_OFF

PATCH In review Aditya Srivastava 2 participants 4 messages Nov 10, 2020

Subsystems (3)
Documentation Checkpatch Documentation process
lore
Share & tools
lore

Aditya Srivastava

author
#0 lore raw

PATCH 1/1 checkpatch: add fix option for MISSING_SIGN_OFF

Currently checkpatch warns us if there is no 'Signed-off-by' line for the patch.

E.g., running checkpatch on commit 9ac060a708e0 ("leaking_addresses:
Completely remove --version flag") reports this error:

ERROR: Missing Signed-off-by: line(s)

Provide a fix by adding a Signed-off-by line corresponding to the author of the patch before the patch separator line. Also avoid this error for the commits where some typo is present in the sign off.

E.g. for commit 8cde5d5f7361 ("bus: ti-sysc: Detect omap4 type timers for quirk") we get missing sign off as well as bad sign off for:

Siganed-off-by Tony Lindgren <[email protected]>

Here it is probably best to give BAD_SIGN_OFF warning for Non-standard signature and avoid MISSING_SIGN_OFF

A quick evaluation over v4.13..v5.8 found out that 4 out of 11 cases for this error is because of missing sign offs in reverts. Add documentation regarding it for the future users.

Suggested-by Joe Perches <[email protected]>
Signed-off-by Aditya Srivastava <[email protected]>
---
 Documentation/process/submitting-patches.rst |  2 ++
 scripts/checkpatch.pl                        | 17 ++++++++++++++---
 2 files changed, 16 insertions(+), 3 deletions(-)
Documentation/process/submitting-patches.rst +2 -0
index 83d9a82055a7..ff065e1f6a25 100644
@@ -404,6 +404,8 @@ then you just add a line saying::
 
 using your real name (sorry, no pseudonyms or anonymous contributions.)
 This will be done for you automatically if you use ``git commit -s``.
+Also reverts should include a Signed-off-by. ``git revert -s`` does
+that for you.
 
 Some people also put extra tags at the end.  They'll just be ignored for
 now, but you can do this to mark internal company procedures or just
scripts/checkpatch.pl +14 -3
index cb46288127ac..849e4a510767 100755
@@ -2404,6 +2404,8 @@ sub process {
 
 	my $last_blank_line = 0;
 	my $last_coalesced_string_linenr = -1;
+	my $patch_separator_linenr = 0;
+	my $is_signed_off = 0;
 
 	our @report = ();
 	our $cnt_lines = 0;

@@ -2755,6 +2757,10 @@ sub process {
 		if ($line =~ /^---$/) {
 			$has_patch_separator = 1;
 			$in_commit_log = 0;
+			# to add missing sign off line before diff(s)
+			if($patch_separator_linenr == 0) {
+				$patch_separator_linenr = $fixlinenr;
+			}
 		}
 
 # Check if MAINTAINERS is being updated.  If so, there's probably no need to

@@ -2771,6 +2777,8 @@ sub process {
 			my $space_after = $3;
 			my $email = $4;
 			my $ucfirst_sign_off = ucfirst(lc($sign_off));
+			# to avoid missing_sign_off error as it most probably is a typo
+			$is_signed_off = 1;
 
 			if ($sign_off !~ /$signature_tags/) {
 				WARN("BAD_SIGN_OFF",

@@ -7118,9 +7126,12 @@ sub process {
 		      "Does not appear to be a unified-diff format patch\n");
 	}
 	if ($is_patch && $has_commit_log && $chk_signoff) {
-		if ($signoff == 0) {
-			ERROR("MISSING_SIGN_OFF",
-			      "Missing Signed-off-by: line(s)\n");
+		if ($signoff == 0 && !$is_signed_off) {
+			if (ERROR("MISSING_SIGN_OFF",
+				  "Missing Signed-off-by: line(s)\n") &&
+			    $fix) {
+				fix_insert_line($patch_separator_linenr, "Signed-off-by: $author");
+			}
 		} elsif ($authorsignoff != 1) {
 			# authorsignoff values:
 			# 0 -> missing sign off
Signature · 6 lines
2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
#1 lore raw
Quoted 21 lines · Aditya Srivastava

Currently checkpatch warns us if there is no 'Signed-off-by' line for the patch.

E.g., running checkpatch on commit 9ac060a708e0 ("leaking_addresses:
Completely remove --version flag") reports this error:

ERROR: Missing Signed-off-by: line(s)

Provide a fix by adding a Signed-off-by line corresponding to the author of the patch before the patch separator line. Also avoid this error for the commits where some typo is present in the sign off.

E.g. for commit 8cde5d5f7361 ("bus: ti-sysc: Detect omap4 type timers for quirk") we get missing sign off as well as bad sign off for:

Siganed-off-by Tony Lindgren <[email protected]>

Here it is probably best to give BAD_SIGN_OFF warning for Non-standard signature and avoid MISSING_SIGN_OFF

Quoted 6 lines

A quick evaluation over v4.13..v5.8 found out that 4 out of 11 cases for this error is because of missing sign offs in reverts. Add documentation regarding it for the future users.

Suggested-by Joe Perches <[email protected]>
Signed-off-by Aditya Srivastava <[email protected]>

Split this patch into two patches. One for documentation and one for the checkpatch.pl change.

The documentation patch needs its own commit message to explain why we think this should be pointed out.

Quoted 32 lines
---
 Documentation/process/submitting-patches.rst |  2 ++
 scripts/checkpatch.pl                        | 17 ++++++++++++++---
 2 files changed, 16 insertions(+), 3 deletions(-)
Documentation/process/submitting-patches.rst +2 -0
index 83d9a82055a7..ff065e1f6a25 100644
@@ -404,6 +404,8 @@ then you just add a line saying::
 
 using your real name (sorry, no pseudonyms or anonymous contributions.)
 This will be done for you automatically if you use ``git commit -s``.
+Also reverts should include a Signed-off-by. ``git revert -s`` does
+that for you.
 
 Some people also put extra tags at the end.  They'll just be ignored for
 now, but you can do this to mark internal company procedures or just
scripts/checkpatch.pl +2 -0
index cb46288127ac..849e4a510767 100755
@@ -2404,6 +2404,8 @@ sub process {
 
        my $last_blank_line = 0;
        my $last_coalesced_string_linenr = -1;
+       my $patch_separator_linenr = 0;
+       my $is_signed_off = 0;
 
        our @report = ();
        our $cnt_lines = 0;

scripts/checkpatch.pl sub process { · line 2763

Quoted 7 lines
scripts/checkpatch.pl +4 -0
@@ -2755,6 +2757,10 @@ sub process {
                if ($line =~ /^---$/) {
                        $has_patch_separator = 1;
                        $in_commit_log = 0;
+                       # to add missing sign off line before diff(s)
+                       if($patch_separator_linenr == 0) {
+                               $patch_separator_linenr = $fixlinenr;
+                       }

Why do you use fixlinenr here? How is that different from linenr?

scripts/checkpatch.pl sub process { · line 2781

Quoted 9 lines
scripts/checkpatch.pl +2 -0
                }
 
 # Check if MAINTAINERS is being updated.  If so, there's probably no need to

@@ -2771,6 +2777,8 @@ sub process {
                        my $space_after = $3;
                        my $email = $4;
                        my $ucfirst_sign_off = ucfirst(lc($sign_off));
+                       # to avoid missing_sign_off error as it most probably is a typo
+                       $is_signed_off = 1;

Probably is_signed_off is the wrong name here.

Other than that, this change looks good.

Check if the patch applies on top of Dwaipayan's latest patches he sent.

Quoted 20 lines
scripts/checkpatch.pl +6 -4
                        if ($sign_off !~ /$signature_tags/) {
                                WARN("BAD_SIGN_OFF",

@@ -7118,9 +7126,12 @@ sub process {
                      "Does not appear to be a unified-diff format patch\n");
        }
        if ($is_patch && $has_commit_log && $chk_signoff) {
-               if ($signoff == 0) {
-                       ERROR("MISSING_SIGN_OFF",
-                             "Missing Signed-off-by: line(s)\n");
+               if ($signoff == 0 && !$is_signed_off) {
+                       if (ERROR("MISSING_SIGN_OFF",
+                                 "Missing Signed-off-by: line(s)\n") &&
+                           $fix) {
+                               fix_insert_line($patch_separator_linenr, "Signed-off-by: $author");
+                       }
                } elsif ($authorsignoff != 1) {
                        # authorsignoff values:
                        # 0 -> missing sign off
--
Quoted 2 lines

2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

author
#2 lore raw
Quoted 26 lines · Lukas Bulwahn
Quoted 25 lines · Aditya Srivastava

Currently checkpatch warns us if there is no 'Signed-off-by' line for the patch.

E.g., running checkpatch on commit 9ac060a708e0 ("leaking_addresses:
Completely remove --version flag") reports this error:

ERROR: Missing Signed-off-by: line(s)

Provide a fix by adding a Signed-off-by line corresponding to the author of the patch before the patch separator line. Also avoid this error for the commits where some typo is present in the sign off.

E.g. for commit 8cde5d5f7361 ("bus: ti-sysc: Detect omap4 type timers for quirk") we get missing sign off as well as bad sign off for:

Siganed-off-by Tony Lindgren <[email protected]>

Here it is probably best to give BAD_SIGN_OFF warning for Non-standard signature and avoid MISSING_SIGN_OFF

A quick evaluation over v4.13..v5.8 found out that 4 out of 11 cases for this error is because of missing sign offs in reverts. Add documentation regarding it for the future users.

Quoted 9 lines
Quoted 2 lines
Suggested-by Joe Perches <[email protected]>
Signed-off-by Aditya Srivastava <[email protected]>

Split this patch into two patches. One for documentation and one for the checkpatch.pl change.

The documentation patch needs its own commit message to explain why we think this should be pointed out.

Okay

Quoted 29 lines
Quoted 29 lines
---
 Documentation/process/submitting-patches.rst |  2 ++
 scripts/checkpatch.pl                        | 17 ++++++++++++++---
 2 files changed, 16 insertions(+), 3 deletions(-)
Documentation/process/submitting-patches.rst +2 -0
index 83d9a82055a7..ff065e1f6a25 100644
@@ -404,6 +404,8 @@ then you just add a line saying::
 
 using your real name (sorry, no pseudonyms or anonymous contributions.)
 This will be done for you automatically if you use ``git commit -s``.
+Also reverts should include a Signed-off-by. ``git revert -s`` does
+that for you.
 
 Some people also put extra tags at the end.  They'll just be ignored for
 now, but you can do this to mark internal company procedures or just
scripts/checkpatch.pl +2 -0
index cb46288127ac..849e4a510767 100755
@@ -2404,6 +2404,8 @@ sub process {
 
        my $last_blank_line = 0;
        my $last_coalesced_string_linenr = -1;
+       my $patch_separator_linenr = 0;
+       my $is_signed_off = 0;
Quoted 13 lines

scripts/checkpatch.pl sub process { · line 2763

Quoted 10 lines
scripts/checkpatch.pl +4 -0
        our @report = ();
        our $cnt_lines = 0;

@@ -2755,6 +2757,10 @@ sub process {
                if ($line =~ /^---$/) {
                        $has_patch_separator = 1;
                        $in_commit_log = 0;
+                       # to add missing sign off line before diff(s)
+                       if($patch_separator_linenr == 0) {
+                               $patch_separator_linenr = $fixlinenr;
+                       }

Why do you use fixlinenr here? How is that different from linenr?

I used $fixlinenr here as we need to later use it for fix_insert_line(), where we are passing fixlinenrs' always. So, to keep it consistent, I used $fixlinenr
Should I use $linenr instead? I think they differ by 1

Quoted 12 lines

scripts/checkpatch.pl sub process { · line 2781

Quoted 9 lines
scripts/checkpatch.pl +2 -0
                }
 
 # Check if MAINTAINERS is being updated.  If so, there's probably no need to

@@ -2771,6 +2777,8 @@ sub process {
                        my $space_after = $3;
                        my $email = $4;
                        my $ucfirst_sign_off = ucfirst(lc($sign_off));
+                       # to avoid missing_sign_off error as it most probably is a typo
+                       $is_signed_off = 1;

Probably is_signed_off is the wrong name here.

Quoted 2 lines

Other than that, this change looks good.

Okay

Quoted 2 lines

Check if the patch applies on top of Dwaipayan's latest patches he sent.

Yes, I have made the changes after applying this patch:
https://lore.kernel.org/linux-kernel-mentees/[email protected]/

Quoted 14 lines
Quoted 14 lines
scripts/checkpatch.pl +4 -3
                        if ($sign_off !~ /$signature_tags/) {
                                WARN("BAD_SIGN_OFF",

@@ -7118,9 +7126,12 @@ sub process {
                      "Does not appear to be a unified-diff format patch\n");
        }
        if ($is_patch && $has_commit_log && $chk_signoff) {
-               if ($signoff == 0) {
-                       ERROR("MISSING_SIGN_OFF",
-                             "Missing Signed-off-by: line(s)\n");
+               if ($signoff == 0 && !$is_signed_off) {
+                       if (ERROR("MISSING_SIGN_OFF",
+                                 "Missing Signed-off-by: line(s)\n") &&
+                           $fix) {
Quoted 8 lines
Quoted 8 lines
scripts/checkpatch.pl +2 -1
+                               fix_insert_line($patch_separator_linenr, "Signed-off-by: $author");
+                       }
                } elsif ($authorsignoff != 1) {
                        # authorsignoff values:
                        # 0 -> missing sign off
--

2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

#3 lore raw
Quoted 81 lines · Aditya
Quoted 26 lines · Lukas Bulwahn
Quoted 25 lines · Aditya Srivastava

Currently checkpatch warns us if there is no 'Signed-off-by' line for the patch.

E.g., running checkpatch on commit 9ac060a708e0 ("leaking_addresses:
Completely remove --version flag") reports this error:

ERROR: Missing Signed-off-by: line(s)

Provide a fix by adding a Signed-off-by line corresponding to the author of the patch before the patch separator line. Also avoid this error for the commits where some typo is present in the sign off.

E.g. for commit 8cde5d5f7361 ("bus: ti-sysc: Detect omap4 type timers for quirk") we get missing sign off as well as bad sign off for:

Siganed-off-by Tony Lindgren <[email protected]>

Here it is probably best to give BAD_SIGN_OFF warning for Non-standard signature and avoid MISSING_SIGN_OFF

A quick evaluation over v4.13..v5.8 found out that 4 out of 11 cases for this error is because of missing sign offs in reverts. Add documentation regarding it for the future users.

Quoted 9 lines
Quoted 2 lines
Suggested-by Joe Perches <[email protected]>
Signed-off-by Aditya Srivastava <[email protected]>

Split this patch into two patches. One for documentation and one for the checkpatch.pl change.

The documentation patch needs its own commit message to explain why we think this should be pointed out.

Okay

Quoted 29 lines
Quoted 29 lines
---
 Documentation/process/submitting-patches.rst |  2 ++
 scripts/checkpatch.pl                        | 17 ++++++++++++++---
 2 files changed, 16 insertions(+), 3 deletions(-)
Documentation/process/submitting-patches.rst +2 -0
index 83d9a82055a7..ff065e1f6a25 100644
@@ -404,6 +404,8 @@ then you just add a line saying::
 
 using your real name (sorry, no pseudonyms or anonymous contributions.)
 This will be done for you automatically if you use ``git commit -s``.
+Also reverts should include a Signed-off-by. ``git revert -s`` does
+that for you.
 
 Some people also put extra tags at the end.  They'll just be ignored for
 now, but you can do this to mark internal company procedures or just
scripts/checkpatch.pl +2 -0
index cb46288127ac..849e4a510767 100755
@@ -2404,6 +2404,8 @@ sub process {
 
        my $last_blank_line = 0;
        my $last_coalesced_string_linenr = -1;
+       my $patch_separator_linenr = 0;
+       my $is_signed_off = 0;
Quoted 13 lines

scripts/checkpatch.pl sub process { · line 2763

Quoted 10 lines
scripts/checkpatch.pl +4 -0
        our @report = ();
        our $cnt_lines = 0;

@@ -2755,6 +2757,10 @@ sub process {
                if ($line =~ /^---$/) {
                        $has_patch_separator = 1;
                        $in_commit_log = 0;
+                       # to add missing sign off line before diff(s)
+                       if($patch_separator_linenr == 0) {
+                               $patch_separator_linenr = $fixlinenr;
+                       }

Why do you use fixlinenr here? How is that different from linenr?

Quoted 5 lines

I used $fixlinenr here as we need to later use it for fix_insert_line(), where we are passing fixlinenrs' always. So, to keep it consistent, I used $fixlinenr
Should I use $linenr instead? I think they differ by 1

I do not know. I am just asking if you are sure that you record the fixlinenr there or not.

fixlinenr sounds good, but I am not sure.

Quoted 12 lines
Quoted 12 lines

scripts/checkpatch.pl sub process { · line 2781

Quoted 9 lines
scripts/checkpatch.pl +2 -0
                }
 
 # Check if MAINTAINERS is being updated.  If so, there's probably no need to

@@ -2771,6 +2777,8 @@ sub process {
                        my $space_after = $3;
                        my $email = $4;
                        my $ucfirst_sign_off = ucfirst(lc($sign_off));
+                       # to avoid missing_sign_off error as it most probably is a typo
+                       $is_signed_off = 1;

Probably is_signed_off is the wrong name here.

Quoted 9 lines
Quoted 2 lines

Other than that, this change looks good.

Okay

Quoted 2 lines

Check if the patch applies on top of Dwaipayan's latest patches he sent.

Yes, I have made the changes after applying this patch:
https://lore.kernel.org/linux-kernel-mentees/[email protected]/

Sounds good. Mention that in the patch below the "---" so that everyone knows.

Lukas
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees