[Linux-kernel-mentees] [PATCH] checkpatch: add fixes for BAD_SIGN_OFF

Lukas Bulwahn lukas.bulwahn at gmail.com
Tue Nov 3 13:39:32 UTC 2020


On Tue, Nov 3, 2020 at 2:31 PM Aditya Srivastava <yashsri421 at gmail.com> wrote:
>
> Currently, checkpatch warns if the author signs-off two or more times
> for the same role.
> E.g., for commit 6dd47d9754ff ("mac80211: fix
> ieee80211_txq_setup_flows() failure path") we get warning:
>
> WARNING: Duplicate signature
> Signed-off-by: Johannes Berg <johannes.berg at intel.com>
>
> Here there are two Signed-off-by lines present for the same user. So,
> we get a warning to remove one.
>
> Similarly, we get warning if the author of the commit signs-off under
> co-developed-by.
> E.g. for commit 6e88559470f5 ("Documentation: Add section about
> CPU vulnerabilities for Spectre") we get:
>
> WARNING: Co-developed-by: should not be used to attribute nominal
> patch author 'Tim Chen <tim.c.chen at linux.intel.com>'
> Co-developed-by: Tim Chen <tim.c.chen at linux.intel.com>
>
> Provide fixes by removing the duplicate signature line and the
> co-developed-by line from the commit
>

A patch shall only do ONE thing. I see two here.

The duplicate signature fix is simply wrong, because the indication of
the duplicate signature is imprecise.

Probably, we would first really need to understand the valid cases of
duplicate signature warnings compared to the invalid ones.

Please rework.


Lukas

> Signed-off-by: Aditya Srivastava <yashsri421 at gmail.com>
> ---
>  scripts/checkpatch.pl | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 58095d9d8f34..4e6cbca495eb 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2818,8 +2818,11 @@ sub process {
>                         $sig_nospace =~ s/\s//g;
>                         $sig_nospace = lc($sig_nospace);
>                         if (defined $signatures{$sig_nospace}) {
> -                               WARN("BAD_SIGN_OFF",
> -                                    "Duplicate signature\n" . $herecurr);
> +                               if (WARN("BAD_SIGN_OFF",
> +                                        "Duplicate signature\n" . $herecurr) &&
> +                                   $fix) {
> +                                       fix_delete_line($fixlinenr, $rawline);
> +                               }
>                         } else {
>                                 $signatures{$sig_nospace} = 1;
>                         }
> @@ -2827,8 +2830,11 @@ sub process {
>  # Check Co-developed-by: immediately followed by Signed-off-by: with same name and email
>                         if ($sign_off =~ /^co-developed-by:$/i) {
>                                 if ($email eq $author) {
> -                                       WARN("BAD_SIGN_OFF",
> -                                             "Co-developed-by: should not be used to attribute nominal patch author '$author'\n" . "$here\n" . $rawline);
> +                                       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);
> +                                       }
>                                 }
>                                 if (!defined $lines[$linenr]) {
>                                         WARN("BAD_SIGN_OFF",
> --
> 2.17.1
>


More information about the Linux-kernel-mentees mailing list