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

Lukas Bulwahn lukas.bulwahn at gmail.com
Tue Nov 10 10:31:47 UTC 2020


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.

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

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

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


More information about the Linux-kernel-mentees mailing list