Re: [Linux-kernel-mentees] [PATCH] checkpatch: fix author Signed-off-by warning for split From: header

[Lukas Bulwahn <lukas.bulwahn@gmail.com>]

For the patch subject line, I actually think this patch is a new feature 
or extension, not a fix. It was not broken, just not supported before.

So maybe: extend author Signed-off-by check for split From: header


On Fri, 18 Sep 2020, Dwaipayan Ray wrote:

> Checkpatch did not handle cases where the author From: header was
> split into two lines. In those cases the author string went empty,
> and checkpatch generated a false missing author signed-off-by
> warning.
> 
> This patch adds support for split From: headers and resolves those
> false warnings.
>

You can drop 'This patch adds'. We see it is a patch, and we see that it 
adds something. Just use imperative tense:

Support split From: headers in AUTHOR_SIGN_OFF check.

(That is a good commit message header as well.)

Can you provide some statistics on number of warnings before and after
and maybe even in more detail, how many of the warnings disappeared with:

  Missing Signed-off-by: line by nominal patch author ''

Probably even new warnings appeared?

> Signed-off-by: Dwaipayan Ray <dwaipayanray1@gmail.com>
> ---
>  scripts/checkpatch.pl | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 504d2e431c60..8c4119ca7d17 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2347,6 +2347,7 @@ sub process {
>  	my $signoff = 0;
>  	my $author = '';
>  	my $authorsignoff = 0;
> +	my $prevheader = 0;
>  	my $is_patch = 0;
>  	my $is_binding_patch = -1;
>  	my $in_header_lines = $file ? 0 : 1;
> @@ -2658,12 +2659,22 @@ sub process {
>  			}
>  		}
>  
> +# Check the patch for a split From:
> +		if ($author eq '' && decode("MIME-Header", $prevheader) =~ /^From:\s*(.*)/) {

How about extending to check if $prevheader is not 0?

> +			$author = $1.$line;
> +			$author = encode("utf8", $author) if ($prevheader =~ /=\?utf-8\?/i);
> +			$author =~ s/"//g;
> +			$author = reformat_email($author);
> +			$prevheader = '';
> +		}
> +
>  # Check the patch for a From:
>  		if (decode("MIME-Header", $line) =~ /^From:\s*(.*)/) {
>  			$author = $1;
>  			$author = encode("utf8", $author) if ($line =~ /=\?utf-8\?/i);
>  			$author =~ s/"//g;
>  			$author = reformat_email($author);
> +			$prevheader = $line;
>  		}
>

So here we see two almost identical parts of code now, right?

Either use a small function or restructure the code such that the 
differences are in two branches and the common code is part of one common 
control flow. You are a good programmer, you can figure this out.

Generally looks good. Let us know once you think it is ready to be tested :)

We got some checkpatch.pl evaluation experts here and I am sure they are 
all happy to test your change and see the evaluation get better.

Lukas
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees