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

Lukas Bulwahn lukas.bulwahn at gmail.com
Tue Nov 10 12:38:52 UTC 2020


On Tue, Nov 10, 2020 at 12:11 PM Aditya <yashsri421 at gmail.com> wrote:
>
> 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
>

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.

> >>                 }
> >>
> >>  # 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/
>

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

Lukas


More information about the Linux-kernel-mentees mailing list