[Linux-kernel-mentees] [PATCH v2] checkpatch: add fix option for MISSING_SIGN_OFF

[Linux-kernel-mentees] [PATCH v2] checkpatch: add fix option for MISSING_SIGN_OFF

[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 <tony@atomide.com>

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

Suggested-by: Joe Perches <joe@perches.com>
Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
---
Changes in v2:
Add space after 'if'
Add check for $patch_separator_linenr to be greater than 0

 scripts/checkpatch.pl | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index cb46288127ac..ac7e5ac80b58 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2404,6 +2404,8 @@ sub process {
 
 	my $last_blank_line = 0;
 	my $last_coalesced_string_linenr = -1;
+	my $patch_separator_linenr = 0;
+	my $non_standard_signature = 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 = $linenr;
+			}
 		}
 
 # Check if MAINTAINERS is being updated.  If so, there's probably no need to
@@ -2775,6 +2781,9 @@ sub process {
 			if ($sign_off !~ /$signature_tags/) {
 				WARN("BAD_SIGN_OFF",
 				     "Non-standard signature: $sign_off\n" . $herecurr);
+
+				# to avoid missing_sign_off error as it most probably is just a typo
+				$non_standard_signature = 1;
 			}
 			if (defined $space_before && $space_before ne "") {
 				if (WARN("BAD_SIGN_OFF",
@@ -7118,9 +7127,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 && !$non_standard_signature) {
+			if (ERROR("MISSING_SIGN_OFF",
+				  "Missing Signed-off-by: line(s)\n") &&
+			    $fix && $patch_separator_linenr > 0) {
+				fix_insert_line($patch_separator_linenr - 1, "Signed-off-by: $author");
+			}
 		} elsif ($authorsignoff != 1) {
 			# authorsignoff values:
 			# 0 -> missing sign off
-- 
2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: add fix option for MISSING_SIGN_OFF

[Lukas Bulwahn]

On Wed, Nov 11, 2020 at 10:01 AM Aditya Srivastava <yashsri421@gmail.com> wrote:
>
> 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 <tony@atomide.com>
>
> Here it is probably best to give BAD_SIGN_OFF warning for Non-standard
> signature and avoid MISSING_SIGN_OFF
>
> Suggested-by: Joe Perches <joe@perches.com>
> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
> ---
> Changes in v2:
> Add space after 'if'
> Add check for $patch_separator_linenr to be greater than 0
>
>  scripts/checkpatch.pl | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index cb46288127ac..ac7e5ac80b58 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2404,6 +2404,8 @@ sub process {
>
>         my $last_blank_line = 0;
>         my $last_coalesced_string_linenr = -1;
> +       my $patch_separator_linenr = 0;
> +       my $non_standard_signature = 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 = $linenr;
> +                       }
>                 }
>
>  # Check if MAINTAINERS is being updated.  If so, there's probably no need to
> @@ -2775,6 +2781,9 @@ sub process {
>                         if ($sign_off !~ /$signature_tags/) {
>                                 WARN("BAD_SIGN_OFF",
>                                      "Non-standard signature: $sign_off\n" . $herecurr);
> +
> +                               # to avoid missing_sign_off error as it most probably is just a typo
> +                               $non_standard_signature = 1;
>                         }
>                         if (defined $space_before && $space_before ne "") {
>                                 if (WARN("BAD_SIGN_OFF",
> @@ -7118,9 +7127,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 && !$non_standard_signature) {
> +                       if (ERROR("MISSING_SIGN_OFF",
> +                                 "Missing Signed-off-by: line(s)\n") &&
> +                           $fix && $patch_separator_linenr > 0) {
> +                               fix_insert_line($patch_separator_linenr - 1, "Signed-off-by: $author");
> +                       }

Maybe I am already digging too much in the details... however:

I think it should still warn about a Missing Signed-off-by: even when
we know there is a $non_standard_signature. So, checkpatch simply
emits two warnings; that is okay in that case.

It is just that our evaluation shows that the provided fix option
should not be suggested when there is a $non_standard_signature
because we actually would predict that there is typo in the intended
Signed-off-by tag and the fix that checkpatch would suggest would not
be adequate.

Joe, what is your opinion?

Aditya, it should not be too difficult to implement the rule that way, right?


>                 } elsif ($authorsignoff != 1) {
>                         # authorsignoff values:
>                         # 0 -> missing sign off
> --
> 2.17.1
>
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: add fix option for MISSING_SIGN_OFF

[Aditya]

On 11/11/20 4:00 pm, Lukas Bulwahn wrote:
> On Wed, Nov 11, 2020 at 10:01 AM Aditya Srivastava <yashsri421@gmail.com> wrote:
>>
>> 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 <tony@atomide.com>
>>
>> Here it is probably best to give BAD_SIGN_OFF warning for Non-standard
>> signature and avoid MISSING_SIGN_OFF
>>
>> Suggested-by: Joe Perches <joe@perches.com>
>> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
>> ---
>> Changes in v2:
>> Add space after 'if'
>> Add check for $patch_separator_linenr to be greater than 0
>>
>>  scripts/checkpatch.pl | 18 +++++++++++++++---
>>  1 file changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index cb46288127ac..ac7e5ac80b58 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -2404,6 +2404,8 @@ sub process {
>>
>>         my $last_blank_line = 0;
>>         my $last_coalesced_string_linenr = -1;
>> +       my $patch_separator_linenr = 0;
>> +       my $non_standard_signature = 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 = $linenr;
>> +                       }
>>                 }
>>
>>  # Check if MAINTAINERS is being updated.  If so, there's probably no need to
>> @@ -2775,6 +2781,9 @@ sub process {
>>                         if ($sign_off !~ /$signature_tags/) {
>>                                 WARN("BAD_SIGN_OFF",
>>                                      "Non-standard signature: $sign_off\n" . $herecurr);
>> +
>> +                               # to avoid missing_sign_off error as it most probably is just a typo
>> +                               $non_standard_signature = 1;
>>                         }
>>                         if (defined $space_before && $space_before ne "") {
>>                                 if (WARN("BAD_SIGN_OFF",
>> @@ -7118,9 +7127,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 && !$non_standard_signature) {
>> +                       if (ERROR("MISSING_SIGN_OFF",
>> +                                 "Missing Signed-off-by: line(s)\n") &&
>> +                           $fix && $patch_separator_linenr > 0) {
>> +                               fix_insert_line($patch_separator_linenr - 1, "Signed-off-by: $author");
>> +                       }
> 
> Maybe I am already digging too much in the details... however:
> 
> I think it should still warn about a Missing Signed-off-by: even when
> we know there is a $non_standard_signature. So, checkpatch simply
> emits two warnings; that is okay in that case.
> 
> It is just that our evaluation shows that the provided fix option
> should not be suggested when there is a $non_standard_signature
> because we actually would predict that there is typo in the intended
> Signed-off-by tag and the fix that checkpatch would suggest would not
> be adequate.
> 
> Joe, what is your opinion?
> 
> Aditya, it should not be too difficult to implement the rule that way, right?
> 

No, I'd probably just have to add the check with $fix, instead of with
$signoff

Thanks
Aditya

> 
>>                 } elsif ($authorsignoff != 1) {
>>                         # authorsignoff values:
>>                         # 0 -> missing sign off
>> --
>> 2.17.1
>>

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: add fix option for MISSING_SIGN_OFF

[Joe Perches]

On Wed, 2020-11-11 at 16:39 +0530, Aditya wrote:
> On 11/11/20 4:00 pm, Lukas Bulwahn wrote:
> > On Wed, Nov 11, 2020 at 10:01 AM Aditya Srivastava <yashsri421@gmail.com> wrote:
> > > 
> > > 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.
[]
> > I think it should still warn about a Missing Signed-off-by: even when
> > we know there is a $non_standard_signature. So, checkpatch simply
> > emits two warnings; that is okay in that case.
> > 
> > It is just that our evaluation shows that the provided fix option
> > should not be suggested when there is a $non_standard_signature
> > because we actually would predict that there is typo in the intended
> > Signed-off-by tag and the fix that checkpatch would suggest would not
> > be adequate.
> > 
> > Joe, what is your opinion?
> > 
> > Aditya, it should not be too difficult to implement the rule that way, right?
> > 
> 
> No, I'd probably just have to add the check with $fix, instead of with
> $signoff

I think it does not matter much which is chosen.

The bad signed-off-by: line would still need to be corrected one
way or another and the added signed-off-line is also possibly
incorrect so it could need to be modified or deleted.


_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: add fix option for MISSING_SIGN_OFF

[Aditya]

On 11/11/20 9:20 pm, Joe Perches wrote:
> On Wed, 2020-11-11 at 16:39 +0530, Aditya wrote:
>> On 11/11/20 4:00 pm, Lukas Bulwahn wrote:
>>> On Wed, Nov 11, 2020 at 10:01 AM Aditya Srivastava <yashsri421@gmail.com> wrote:
>>>>
>>>> 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.
> []
>>> I think it should still warn about a Missing Signed-off-by: even when
>>> we know there is a $non_standard_signature. So, checkpatch simply
>>> emits two warnings; that is okay in that case.
>>>
>>> It is just that our evaluation shows that the provided fix option
>>> should not be suggested when there is a $non_standard_signature
>>> because we actually would predict that there is typo in the intended
>>> Signed-off-by tag and the fix that checkpatch would suggest would not
>>> be adequate.
>>>
>>> Joe, what is your opinion?
>>>
>>> Aditya, it should not be too difficult to implement the rule that way, right?
>>>
>>
>> No, I'd probably just have to add the check with $fix, instead of with
>> $signoff
> 
> I think it does not matter much which is chosen.
> 
> The bad signed-off-by: line would still need to be corrected one
> way or another and the added signed-off-line is also possibly
> incorrect so it could need to be modified or deleted.
> 
> 

I think I might have misunderstood here that I do not need to make
changes. Just confirming, Do I need to modify the patch?
Pardon me for my late attention to it.

Thanks
Aditya
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees