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

[Aditya]

On 10/11/20 8:52 pm, Lukas Bulwahn wrote:
> On Tue, Nov 10, 2020 at 4:13 PM Aditya <yashsri421 at gmail.com> wrote:
>>
>> On 10/11/20 7:30 pm, Lukas Bulwahn wrote:
>>> On Tue, Nov 10, 2020 at 2:06 PM 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
>>>>
>>>> Suggested-by: Joe Perches <joe at perches.com>
>>>> Signed-off-by: Aditya Srivastava <yashsri421 at gmail.com>
>>>> ---
>>>> This patch was made after applying https://lore.kernel.org/linux-kernel-mentees/20201108100632.75340-1-dwaipayanray1@gmail.com/
>>>> and my last changes at https://lore.kernel.org/linux-kernel-mentees/20201108134317.25400-1-yashsri421@gmail.com/
>>>>
>>>>  scripts/checkpatch.pl | 18 +++++++++++++++---
>>>>  1 file changed, 15 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>>>> index cb46288127ac..2deffd0c091b 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;
>>>
>>> Well are you sure linenr is correct? (As I wrote, I do not know, but
>>> you should know.)
>>>
>>> I think you would need to create a patch for testing your feature
>>> where multiple things are fixed within the same patch and then check
>>> if the fix is added at the right place.
>>>
>>
>> I checked the fix over following patch.
>>
>> I have modified the patch such that it has co-developed-by as the
>> author(nominal patch author warning, which requires this line to be
>> deleted)
>>
>> Also 'occuring', which is a typo(where the typo needs to be replaced)
>>
>> It works as expected.
>>
> 
> Well to really check if it works as expected, you need to have a test
> patch where a fix adds multiple lines into the fixed patch before the
> line you try to record.
> 
> Then, fixlinenr and linenr are probably more than just 1 line apart,
> but really multiple lines apart and only if you record the right
> variable it really works.
> 

Actually we currently don't have any new line insertion fixes for
lines above patch separator.
So, I modified checkpatch a bit to insert (instead of delete) for
co-developed-by.
ie here:

if (WARN("BAD_SIGN_OFF",
	 "Co-developed-by: should not be used to attribute nominal patch
author '$author'\n" . "$here\n" . $rawline) &&
    $fix) {
-	fix_delete_line($fixlinenr, $rawline);
+	fix_insert_line($fixlinenr, $rawline);
}


When repeated a few times and removing signed-off-by, this provided an
experience of inserting multiple lines before signed-off-by (as for
each co-developed-by, it is inserting a new co-developed-by)

With this as well, the fix was working as expected, ie, got this after
some iterations of removing signed-off-by and keeping co-developed-by:

...
It is causing boot failures with qemu mac99 in at least some
configurations.occurring
Co-developed-by: Michael Ellerman <mpe at ellerman.id.au>
Co-developed-by: Michael Ellerman <mpe at ellerman.id.au>
Co-developed-by: Michael Ellerman <mpe at ellerman.id.au>
Co-developed-by: Michael Ellerman <mpe at ellerman.id.au>
Co-developed-by: Michael Ellerman <mpe at ellerman.id.au>
Co-developed-by: Michael Ellerman <mpe at ellerman.id.au>
Co-developed-by: Michael Ellerman <mpe at ellerman.id.au>
Co-developed-by: Michael Ellerman <mpe at ellerman.id.au>
Co-developed-by: Michael Ellerman <mpe at ellerman.id.au>
Co-developed-by: Michael Ellerman <mpe at ellerman.id.au>
Co-developed-by: Michael Ellerman <mpe at ellerman.id.au>
Co-developed-by: Michael Ellerman <mpe at ellerman.id.au>
Co-developed-by: Michael Ellerman <mpe at ellerman.id.au>
Co-developed-by: Michael Ellerman <mpe at ellerman.id.au>
Co-developed-by: Michael Ellerman <mpe at ellerman.id.au>
Co-developed-by: Michael Ellerman <mpe at ellerman.id.au>
Signed-off-by: Michael Ellerman <mpe at ellerman.id.au>
---
 arch/powerpc/include/asm/book3s/32/hash.h | 8 ++++----


Aditya

> Other than that, it is all good.
> 
> Lukas
>