[PATCH v2] checkpatch: use patch subject when reading from stdin

[PATCH v2] checkpatch: use patch subject when reading from stdin

[Geert Uytterhoeven]

While "git am" can apply an mbox file containing multiple patches (e.g.
as created by b4[1], or a patch bundle downloaded from patchwork),
checkpatch does not have proper support for that.  When operating on an
mbox, checkpatch will merge all detected tags, and complain falsely
about duplicates:

    WARNING: Duplicate signature

As modifying checkpatch to reset state in between each patch is a lot of
work, a simple solution is splitting the mbox into individual patches,
and invoking checkpatch for each of them.  Fortunately checkpatch can read
a patch from stdin, so the classic "formail" tool can be used to split
the mbox, and pipe all individual patches to checkpatch:

    formail -s scripts/checkpatch.pl < my-mbox

However, when reading a patch file from standard input, checkpatch calls
it "Your patch", and reports its state as:

    Your patch has style problems, please review.

or:

    Your patch has no obvious style problems and is ready for submission.

Hence it can be difficult to identify which patches need to be reviewed
and improved.

Fix this by replacing "Your patch" by (the first line of) the email
subject, if present.

Note that "git mailsplit" can also be used to split an mbox, but it will
create individual files for each patch, thus requiring cleanup
afterwards.  Formail does not have this disadvantage.

[1] https://git.kernel.org/pub/scm/utils/b4/b4.git

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - Add more rationale,
  - Refer to the new b4 tool.
---
 scripts/checkpatch.pl | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index eac40f0abd56a9f4..3355358697d9e790 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1057,6 +1057,10 @@ for my $filename (@ARGV) {
 	}
 	while (<$FILE>) {
 		chomp;
+		if ($vname eq 'Your patch') {
+			my ($subject) = $_ =~ /^Subject:\s*(.*)/;
+			$vname = '"' . $subject . '"' if $subject;
+		}
 		push(@rawlines, $_);
 	}
 	close($FILE);
-- 
2.17.1

Re: [PATCH v2] checkpatch: use patch subject when reading from stdin

[Andrew Morton]

On Tue,  5 May 2020 15:26:13 +0200 Geert Uytterhoeven <geert+renesas@glider.be> wrote:

> While "git am" can apply an mbox file containing multiple patches (e.g.
> as created by b4[1], or a patch bundle downloaded from patchwork),
> checkpatch does not have proper support for that.  When operating on an
> mbox, checkpatch will merge all detected tags, and complain falsely
> about duplicates:
> 
>     WARNING: Duplicate signature
> 
> As modifying checkpatch to reset state in between each patch is a lot of
> work, a simple solution is splitting the mbox into individual patches,
> and invoking checkpatch for each of them.  Fortunately checkpatch can read
> a patch from stdin, so the classic "formail" tool can be used to split
> the mbox, and pipe all individual patches to checkpatch:
> 
>     formail -s scripts/checkpatch.pl < my-mbox
> 
> However, when reading a patch file from standard input, checkpatch calls
> it "Your patch", and reports its state as:
> 
>     Your patch has style problems, please review.
> 
> or:
> 
>     Your patch has no obvious style problems and is ready for submission.

Showing the proposed "after patch" output would be helpful.  It seems
that it will be

	"checkpatch: use patch subject when reading from stdin" has no obvious style problems and is ready for submission.

yes?

> Hence it can be difficult to identify which patches need to be reviewed
> and improved.
> 
> Fix this by replacing "Your patch" by (the first line of) the email
> subject, if present.
> 
> Note that "git mailsplit" can also be used to split an mbox, but it will
> create individual files for each patch, thus requiring cleanup
> afterwards.  Formail does not have this disadvantage.
> 

Re: [PATCH v2] checkpatch: use patch subject when reading from stdin

[Geert Uytterhoeven]

Hi Andrew,

On Tue, May 5, 2020 at 9:18 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> On Tue,  5 May 2020 15:26:13 +0200 Geert Uytterhoeven <geert+renesas@glider.be> wrote:
> > While "git am" can apply an mbox file containing multiple patches (e.g.
> > as created by b4[1], or a patch bundle downloaded from patchwork),
> > checkpatch does not have proper support for that.  When operating on an
> > mbox, checkpatch will merge all detected tags, and complain falsely
> > about duplicates:
> >
> >     WARNING: Duplicate signature
> >
> > As modifying checkpatch to reset state in between each patch is a lot of
> > work, a simple solution is splitting the mbox into individual patches,
> > and invoking checkpatch for each of them.  Fortunately checkpatch can read
> > a patch from stdin, so the classic "formail" tool can be used to split
> > the mbox, and pipe all individual patches to checkpatch:
> >
> >     formail -s scripts/checkpatch.pl < my-mbox
> >
> > However, when reading a patch file from standard input, checkpatch calls
> > it "Your patch", and reports its state as:
> >
> >     Your patch has style problems, please review.
> >
> > or:
> >
> >     Your patch has no obvious style problems and is ready for submission.
>
> Showing the proposed "after patch" output would be helpful.  It seems
> that it will be
>
>         "checkpatch: use patch subject when reading from stdin" has no obvious style problems and is ready for submission.
>
> yes?

Almost right:

"[PATCH v2] checkpatch: use patch subject when reading from stdin" has
no obvious style problems and is ready for submission.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2] checkpatch: use patch subject when reading from stdin

[Joe Perches]

On Tue, 2020-05-05 at 15:26 +0200, Geert Uytterhoeven wrote:
> While "git am" can apply an mbox file containing multiple patches (e.g.
> as created by b4[1], or a patch bundle downloaded from patchwork),
> checkpatch does not have proper support for that.  When operating on an
> mbox, checkpatch will merge all detected tags, and complain falsely
> about duplicates:
> 
>     WARNING: Duplicate signature
> 
> As modifying checkpatch to reset state in between each patch is a lot of
> work, a simple solution is splitting the mbox into individual patches,
> and invoking checkpatch for each of them.  Fortunately checkpatch can read
> a patch from stdin, so the classic "formail" tool can be used to split
> the mbox, and pipe all individual patches to checkpatch:
> 
>     formail -s scripts/checkpatch.pl < my-mbox
> 
> However, when reading a patch file from standard input, checkpatch calls
> it "Your patch", and reports its state as:
> 
>     Your patch has style problems, please review.
> 
> or:
> 
>     Your patch has no obvious style problems and is ready for submission.
> 
> Hence it can be difficult to identify which patches need to be reviewed
> and improved.
> 
> Fix this by replacing "Your patch" by (the first line of) the email
> subject, if present.
> 
> Note that "git mailsplit" can also be used to split an mbox, but it will
> create individual files for each patch, thus requiring cleanup
> afterwards.  Formail does not have this disadvantage.
> 
> [1] https://git.kernel.org/pub/scm/utils/b4/b4.git
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v2:
>   - Add more rationale,
>   - Refer to the new b4 tool.
> ---
>  scripts/checkpatch.pl | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index eac40f0abd56a9f4..3355358697d9e790 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1057,6 +1057,10 @@ for my $filename (@ARGV) {
>  	}
>  	while (<$FILE>) {
>  		chomp;
> +		if ($vname eq 'Your patch') {
> +			my ($subject) = $_ =~ /^Subject:\s*(.*)/;
> +			$vname = '"' . $subject . '"' if $subject;
> +		}
>  		push(@rawlines, $_);
>  	}
>  	close($FILE);

There's a less cpu intensive way to do this,
for small patches, on my little laptop it's a
few dozen milliseconds faster, and for very
large patches multiple seconds faster to use
the following patch:

Substitute Geert's patch with the below but:

Acked-by: Joe Perches <joe@perches.com>

---

 scripts/checkpatch.pl | 1 +
 1 file changed, 1 insertion(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index f0092104ff7b..29786a097862 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1062,6 +1062,7 @@ for my $filename (@ARGV) {
 	while (<$FILE>) {
 		chomp;
 		push(@rawlines, $_);
+		$vname = "\"$1\"" if ($filename eq '-' && $_ =~ /^Subject:\s*(.*)/);
 	}
 	close($FILE);
 

Re: [kernel.org users] [PATCH v2] checkpatch: use patch subject when reading from stdin

[Pali Rohár]

Hello!

On Tuesday 05 May 2020 12:57:37 Joe Perches wrote:
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index eac40f0abd56a9f4..3355358697d9e790 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -1057,6 +1057,10 @@ for my $filename (@ARGV) {
> >  	}
> >  	while (<$FILE>) {
> >  		chomp;
> > +		if ($vname eq 'Your patch') {
> > +			my ($subject) = $_ =~ /^Subject:\s*(.*)/;
> > +			$vname = '"' . $subject . '"' if $subject;
> > +		}
> >  		push(@rawlines, $_);
> >  	}
> >  	close($FILE);
> 
> There's a less cpu intensive way to do this,
> for small patches, on my little laptop it's a
> few dozen milliseconds faster, and for very
> large patches multiple seconds faster to use
> the following patch:
> 
> Substitute Geert's patch with the below but:
> 
> Acked-by: Joe Perches <joe@perches.com>
> 
> ---
> 
>  scripts/checkpatch.pl | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index f0092104ff7b..29786a097862 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1062,6 +1062,7 @@ for my $filename (@ARGV) {
>  	while (<$FILE>) {
>  		chomp;
>  		push(@rawlines, $_);
> +		$vname = "\"$1\"" if ($filename eq '-' && $_ =~ /^Subject:\s*(.*)/);

Hint: You can use qq operator to make code more readable (no need to
escape quote character). And maybe you should match Subject as
case-insensitive and expects at least one space after colon.
As a Perl developer I would write above code as:

+		$vname = qq("$1") if $filename eq '-' && $_ =~ m/^Subject:\s+(.+)/i;

Anyway, what would happen if subject line contains quotes?

>  	}
>  	close($FILE);
>  
> 

Re: [kernel.org users] [PATCH v2] checkpatch: use patch subject when reading from stdin

[Joe Perches]

On Tue, 2020-05-05 at 22:40 +0200, Pali Rohár wrote:
> Hello!
> 
> On Tuesday 05 May 2020 12:57:37 Joe Perches wrote:
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > index eac40f0abd56a9f4..3355358697d9e790 100755
> > > --- a/scripts/checkpatch.pl
> > > +++ b/scripts/checkpatch.pl
> > > @@ -1057,6 +1057,10 @@ for my $filename (@ARGV) {
> > >  	}
> > >  	while (<$FILE>) {
> > >  		chomp;
> > > +		if ($vname eq 'Your patch') {
> > > +			my ($subject) = $_ =~ /^Subject:\s*(.*)/;
> > > +			$vname = '"' . $subject . '"' if $subject;
> > > +		}
> > >  		push(@rawlines, $_);
> > >  	}
> > >  	close($FILE);
> > 
> > There's a less cpu intensive way to do this,
> > for small patches, on my little laptop it's a
> > few dozen milliseconds faster, and for very
> > large patches multiple seconds faster to use
> > the following patch:
> > 
> > Substitute Geert's patch with the below but:
> > 
> > Acked-by: Joe Perches <joe@perches.com>
> > 
> > ---
> > 
> >  scripts/checkpatch.pl | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index f0092104ff7b..29786a097862 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -1062,6 +1062,7 @@ for my $filename (@ARGV) {
> >  	while (<$FILE>) {
> >  		chomp;
> >  		push(@rawlines, $_);
> > +		$vname = "\"$1\"" if ($filename eq '-' && $_ =~ /^Subject:\s*(.*)/);
> 
> Hint: You can use qq operator to make code more readable (no need to
> escape quote character). And maybe you should match Subject as
> case-insensitive and expects at least one space after colon.
> As a Perl developer I would write above code as:
> 
> +		$vname = qq("$1") if $filename eq '-' && $_ =~ m/^Subject:\s+(.+)/i;
> 
> Anyway, what would happen if subject line contains quotes?

Hi Pali.

bad things... ;) so your suggestion is better.

But/And checkpatch uses parens for if statements.

cheers and thanks, Joe