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

[Aditya]

On 10/11/20 4:01 pm, Lukas Bulwahn wrote:
> On Tue, Nov 10, 2020 at 11:06 AM Aditya Srivastava <yashsri421 at 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 at atomide.com>
>>
>> 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 <joe at perches.com>
>> Signed-off-by: Aditya Srivastava <yashsri421 at gmail.com>
> 
> 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

>> ---
>>  Documentation/process/submitting-patches.rst |  2 ++
>>  scripts/checkpatch.pl                        | 17 ++++++++++++++---
>>  2 files changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
>> index 83d9a82055a7..ff065e1f6a25 100644
>> --- a/Documentation/process/submitting-patches.rst
>> +++ b/Documentation/process/submitting-patches.rst
>> @@ -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
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index cb46288127ac..849e4a510767 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 $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;
>> +                       }
> 
> 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

>>                 }
>>
>>  # 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.
>
Okay

> 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/20201108100632.75340-1-dwaipayanray1@gmail.com/

>>
>>                         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
>> --
>> 2.17.1
>>