* [PATCH v2] checkpatch: use patch subject when reading from stdin
@ 2020-05-05 13:26 Geert Uytterhoeven
2020-05-05 19:18 ` Andrew Morton
2020-05-05 19:57 ` Joe Perches
0 siblings, 2 replies; 6+ messages in thread
From: Geert Uytterhoeven @ 2020-05-05 13:26 UTC (permalink / raw)
To: Andy Whitcroft, Joe Perches
Cc: Konstantin Ryabitsev, Andrew Morton, linux-kernel, users,
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] checkpatch: use patch subject when reading from stdin
2020-05-05 13:26 [PATCH v2] checkpatch: use patch subject when reading from stdin Geert Uytterhoeven
@ 2020-05-05 19:18 ` Andrew Morton
2020-05-06 6:55 ` Geert Uytterhoeven
2020-05-05 19:57 ` Joe Perches
1 sibling, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2020-05-05 19:18 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Andy Whitcroft, Joe Perches, Konstantin Ryabitsev, linux-kernel, users
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.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] checkpatch: use patch subject when reading from stdin
2020-05-05 13:26 [PATCH v2] checkpatch: use patch subject when reading from stdin Geert Uytterhoeven
2020-05-05 19:18 ` Andrew Morton
@ 2020-05-05 19:57 ` Joe Perches
2020-05-05 20:40 ` [kernel.org users] " Pali Rohár
1 sibling, 1 reply; 6+ messages in thread
From: Joe Perches @ 2020-05-05 19:57 UTC (permalink / raw)
To: Geert Uytterhoeven, Andy Whitcroft
Cc: Konstantin Ryabitsev, Andrew Morton, linux-kernel, users
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);
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [kernel.org users] [PATCH v2] checkpatch: use patch subject when reading from stdin
2020-05-05 19:57 ` Joe Perches
@ 2020-05-05 20:40 ` Pali Rohár
2020-05-05 20:54 ` Joe Perches
0 siblings, 1 reply; 6+ messages in thread
From: Pali Rohár @ 2020-05-05 20:40 UTC (permalink / raw)
To: Joe Perches
Cc: Geert Uytterhoeven, Andy Whitcroft, Konstantin Ryabitsev,
Andrew Morton, linux-kernel, users
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);
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [kernel.org users] [PATCH v2] checkpatch: use patch subject when reading from stdin
2020-05-05 20:40 ` [kernel.org users] " Pali Rohár
@ 2020-05-05 20:54 ` Joe Perches
0 siblings, 0 replies; 6+ messages in thread
From: Joe Perches @ 2020-05-05 20:54 UTC (permalink / raw)
To: Pali Rohár
Cc: Geert Uytterhoeven, Andy Whitcroft, Konstantin Ryabitsev,
Andrew Morton, linux-kernel, users
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] checkpatch: use patch subject when reading from stdin
2020-05-05 19:18 ` Andrew Morton
@ 2020-05-06 6:55 ` Geert Uytterhoeven
0 siblings, 0 replies; 6+ messages in thread
From: Geert Uytterhoeven @ 2020-05-06 6:55 UTC (permalink / raw)
To: Andrew Morton
Cc: Geert Uytterhoeven, Andy Whitcroft, Joe Perches,
Konstantin Ryabitsev, Linux Kernel Mailing List, users
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-05-06 6:55 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-05 13:26 [PATCH v2] checkpatch: use patch subject when reading from stdin Geert Uytterhoeven
2020-05-05 19:18 ` Andrew Morton
2020-05-06 6:55 ` Geert Uytterhoeven
2020-05-05 19:57 ` Joe Perches
2020-05-05 20:40 ` [kernel.org users] " Pali Rohár
2020-05-05 20:54 ` Joe Perches
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.