[PATCH 0/2] checkpatch: Add support for Checkpatch-ignore patch footer

[PATCH 0/2] checkpatch: Add support for Checkpatch-ignore patch footer

[Brendan Jackman]

Checkpatch sometimes has false positives. This makes it less useful for
automatic usage: tools like b4 [0] can run checkpatch on all of your
patches and give you a quick overview. When iterating on a branch, it's
tiresome to manually re-check that any errors are known false positives.

This patch adds a feature to record in the commit message that a patch
might produce a certain checkpatch error, and that this is an expected
false positive. Recording this information in the patch itself can also
highlight it to reviewers, so they can make a judgment as to whether
it's appropriate to ignore.

Signed-off-by: Brendan Jackman <jackmanb@google.com>
---
Brendan Jackman (2):
      checkpatch: Add support for Checkpatch-ignore patch footer
      docs: checkpatch: Document Checkpatch-ignore patch footer

 Documentation/dev-tools/checkpatch.rst |  9 ++++++++-
 scripts/checkpatch.pl                  | 11 ++++++++++-
 2 files changed, 18 insertions(+), 2 deletions(-)
---
base-commit: eae581be230bec00287807017839b3b2cd83f9ff
change-id: 20250113-checkpatch-ignore-1096914844eb

Best regards,
-- 
Brendan Jackman <jackmanb@google.com>

[PATCH 1/2] checkpatch: Add support for Checkpatch-ignore patch footer

[Brendan Jackman]

Checkpatch sometimes has false positives. This makes it less useful for
automatic usage: tools like b4 [0] can run checkpatch on all of your
patches and give you a quick overview. When iterating on a branch, it's
tiresome to manually re-check that any errors are known false positives.

This patch adds a feature to record in the commit message that a patch
might produce a certain checkpatch error, and that this is an expected
false positive. Recording this information in the patch itself can also
highlight it to reviewers, so they can make a judgment as to whether
it's appropriate to ignore.

To avoid significant reworks to the Perl code, this is implemented by
mutating a global variable while processing each patch. (The variable
name refers to a patch as a "file" for consistency with other code).

This feature is immediately adopted for this commit itself, which
falls afoul of EMAIL_SUBJECT due to the word "checkpatch" appearing in
the "Checkpatch-ignore" reference in the title - a good example of a
false positive.

[0] b4 - see "--check" arg
    https://b4.docs.kernel.org/en/latest/contributor/prep.html

Checkpatch-ignore: EMAIL_SUBJECT
Signed-off-by: Brendan Jackman <jackmanb@google.com>
---
 scripts/checkpatch.pl | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 9eed3683ad76caffbbb2418e5dbea7551d374406..1a2ae442068d870903d46bd2dc63b757609e142d 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -53,7 +53,10 @@ my %debug;
 my %camelcase = ();
 my %use_type = ();
 my @use = ();
+# Error types to ignore during the whole invocation.
 my %ignore_type = ();
+# Error types to be ignored in the present "file" (i.e. patch).
+my %file_ignore_type = ();
 my @ignore = ();
 my $help = 0;
 my $configuration_file = ".checkpatch.conf";
@@ -2329,7 +2332,7 @@ sub show_type {
 
 	return defined $use_type{$type} if (scalar keys %use_type > 0);
 
-	return !defined $ignore_type{$type};
+	return !defined $ignore_type{$type} && !defined $file_ignore_type{$type};
 }
 
 sub report {
@@ -2701,6 +2704,8 @@ sub process {
 
 	my $checklicenseline = 1;
 
+	%file_ignore_type = ();
+
 	sanitise_line_reset();
 	my $line;
 	foreach my $rawline (@rawlines) {
@@ -2778,6 +2783,10 @@ sub process {
 		if ($setup_docs && $line =~ /^\+/) {
 			push(@setup_docs, $line);
 		}
+
+		if ($line =~ /^Checkpatch-ignore:\s+(.*)/) {
+			hash_save_array_words(\%file_ignore_type, [$1]);
+		}
 	}
 
 	$prefix = '';

-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH 2/2] docs: checkpatch: Document Checkpatch-ignore patch footer

[Brendan Jackman]

If included in patch descriptions, this will function much like the
--ignore flag.

Checkpatch-ignore: EMAIL_SUBJECT

Signed-off-by: Brendan Jackman <jackmanb@google.com>
---
 Documentation/dev-tools/checkpatch.rst | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
index abb3ff6820766ee0c29112b256bcc44ce41fffba..b1d5616c72029d3d8c8c236cd8d05bb839018c0a 100644
--- a/Documentation/dev-tools/checkpatch.rst
+++ b/Documentation/dev-tools/checkpatch.rst
@@ -10,8 +10,12 @@ also be run on file contexts and without the kernel tree.
 
 Checkpatch is not always right. Your judgement takes precedence over checkpatch
 messages.  If your code looks better with the violations, then its probably
-best left alone.
+best left alone.  If you do that, consider adding the Checkpatch-ignore patch
+footer to record this decision.
 
+For example::
+
+  Checkpatch-ignore: EMAIL_SUBJECT,MACRO_ARG_REUSE
 
 Options
 =======
@@ -114,6 +118,9 @@ Available options:
 
    Checkpatch will not emit messages for the specified types.
 
+   Note that violations can also be permanently disabled using the
+   Checkpatch-ignore patch footer.
+
    Example::
 
      ./scripts/checkpatch.pl mypatch.patch --ignore EMAIL_SUBJECT,BRACES

-- 
2.47.1.613.gc27f4b7a9f-goog

Re: [PATCH 0/2] checkpatch: Add support for Checkpatch-ignore patch footer

[Matthew Wilcox]

On Mon, Jan 13, 2025 at 04:04:21PM +0000, Brendan Jackman wrote:
> Checkpatch sometimes has false positives. This makes it less useful for
> automatic usage: tools like b4 [0] can run checkpatch on all of your
> patches and give you a quick overview. When iterating on a branch, it's
> tiresome to manually re-check that any errors are known false positives.
> 
> This patch adds a feature to record in the commit message that a patch
> might produce a certain checkpatch error, and that this is an expected
> false positive. Recording this information in the patch itself can also
> highlight it to reviewers, so they can make a judgment as to whether
> it's appropriate to ignore.

I think humans should always ignore checkpatch.  It's basically
worthless.

Re: [PATCH 0/2] checkpatch: Add support for Checkpatch-ignore patch footer

[Brendan Jackman]

On Mon, 13 Jan 2025 at 17:20, Matthew Wilcox <willy@infradead.org> wrote:
> I think humans should always ignore checkpatch.  It's basically
> worthless.

Do you know of anything better? I guess it's feasible to build
something that's actually good at this job using tree-sitter or
something, maybe it exists. But outside of projects that just enforce
clang-format or whatever, checkpatch.pl is still the best (or perhaps
least bad) thing I've personally experienced.

I won't deny that most error types have only ever presented themselves
to me as noise. But the basic "don't do braces like that" and "you
have a stray space here" stuff has been useful.

Re: [PATCH 1/2] checkpatch: Add support for Checkpatch-ignore patch footer

[Joe Perches]

On Mon, 2025-01-13 at 16:04 +0000, Brendan Jackman wrote:
> Checkpatch sometimes has false positives. This makes it less useful for
> automatic usage: tools like b4 [0] can run checkpatch on all of your
> patches and give you a quick overview. When iterating on a branch, it's
> tiresome to manually re-check that any errors are known false positives.

If you do this, and perhaps it's not particularly necessary at all,
I suggest using something like the message-id or branch name for an
ignored types file and have the script auto-write the found types
into that file.

Re: [PATCH 1/2] checkpatch: Add support for Checkpatch-ignore patch footer

[Brendan Jackman]

On Mon, 13 Jan 2025, 20:15 Joe Perches, <joe@perches.com> wrote:
>
> On Mon, 2025-01-13 at 16:04 +0000, Brendan Jackman wrote:
> > Checkpatch sometimes has false positives. This makes it less useful for
> > automatic usage: tools like b4 [0] can run checkpatch on all of your
> > patches and give you a quick overview. When iterating on a branch, it's
> > tiresome to manually re-check that any errors are known false positives.
>
> If you do this, and perhaps it's not particularly necessary at all,
> I suggest using something like the message-id or branch name for an
> ignored types file and have the script auto-write the found types
> into that file.

Do you mean to say the problem is better solved in b4 instead of checkpatch?

I think that's a downgrade from the Checkpatch-args approach, because
b4 is just one of many many tools that wrap checkpatch. I think it's
nice to solve the problem for everyone.

Also, having the config in the commit message means it's there for
everyone instead of just the patch author. Running checkpatch on other
people's patches is not something I have much interest in doing
deliberately, but I'm sure there are those who do it. Maybe there are
even maintainers who would like to have their -next branch entirely
checkpatch-clean if that was an option.

Plus I bet there are just cases where it's interesting to know the
difference between "this author doesn't care about checkpatch" and
"this author disagrees with checkpatch on this patch".

Re: [PATCH 1/2] checkpatch: Add support for Checkpatch-ignore patch footer

[Brendan Jackman]

On Tue, 14 Jan 2025 at 12:42, Brendan Jackman <jackmanb@google.com> wrote:
> I think that's a downgrade from the Checkpatch-args approach

(Sorry I mean Checkpatch-ignore of course)

Re: [PATCH 1/2] checkpatch: Add support for Checkpatch-ignore patch footer

[Konstantin Ryabitsev]

On Mon, Jan 13, 2025 at 04:04:22PM +0000, Brendan Jackman wrote:
> Checkpatch sometimes has false positives. This makes it less useful for
> automatic usage: tools like b4 [0] can run checkpatch on all of your
> patches and give you a quick overview. When iterating on a branch, it's
> tiresome to manually re-check that any errors are known false positives.
> 
> This patch adds a feature to record in the commit message that a patch
> might produce a certain checkpatch error, and that this is an expected
> false positive. Recording this information in the patch itself can also
> highlight it to reviewers, so they can make a judgment as to whether
> it's appropriate to ignore.
> 
> To avoid significant reworks to the Perl code, this is implemented by
> mutating a global variable while processing each patch. (The variable
> name refers to a patch as a "file" for consistency with other code).
> 
> This feature is immediately adopted for this commit itself, which
> falls afoul of EMAIL_SUBJECT due to the word "checkpatch" appearing in
> the "Checkpatch-ignore" reference in the title - a good example of a
> false positive.
> 
> [0] b4 - see "--check" arg
>     https://b4.docs.kernel.org/en/latest/contributor/prep.html
> 
> Checkpatch-ignore: EMAIL_SUBJECT
> Signed-off-by: Brendan Jackman <jackmanb@google.com>

Do we really want this to become part of the permanent commit message? I'm
pretty sure this won't go over well with many.

-K

Re: [PATCH 1/2] checkpatch: Add support for Checkpatch-ignore patch footer

[Brendan Jackman]

On Tue, 14 Jan 2025 at 14:34, Konstantin Ryabitsev
<konstantin@linuxfoundation.org> wrote:
> Do we really want this to become part of the permanent commit message? I'm
> pretty sure this won't go over well with many.

Why not?

Re: [PATCH 1/2] checkpatch: Add support for Checkpatch-ignore patch footer

[Konstantin Ryabitsev]

On Tue, Jan 14, 2025 at 03:25:41PM +0100, Brendan Jackman wrote:
> <konstantin@linuxfoundation.org> wrote:
> > Do we really want this to become part of the permanent commit message? I'm
> > pretty sure this won't go over well with many.
> 
> Why not?

Tweaks aimed at checkpatch are only useful during the code review stage, so
once that code is accepted upstream, they become wholly irrelevant. A
checkpatch trailer in the permanent commit record serves no purpose, not even
a historical one.

At best, utility trailers like that need to go into the basement of the patch,
not into the commit message.

-K

Re: [PATCH 1/2] checkpatch: Add support for Checkpatch-ignore patch footer

[Brendan Jackman]

On Tue, 14 Jan 2025 at 17:04, Konstantin Ryabitsev
<konstantin@linuxfoundation.org> wrote:
>
> On Tue, Jan 14, 2025 at 03:25:41PM +0100, Brendan Jackman wrote:
> > <konstantin@linuxfoundation.org> wrote:
> > > Do we really want this to become part of the permanent commit message? I'm
> > > pretty sure this won't go over well with many.
> >
> > Why not?
>
> Tweaks aimed at checkpatch are only useful during the code review stage, so
> once that code is accepted upstream, they become wholly irrelevant. A
> checkpatch trailer in the permanent commit record serves no purpose, not even
> a historical one.

Yeah that's a good argument for them being unnecessary. It's not clear
why them persisting beyond their useful lifetime would be a problem
though. Any given reader of a commit message is already very likely to
see tags they don't care about in that moment, is that something
people really complain about?

> At best, utility trailers like that need to go into the basement of the patch,
> not into the commit message.

If people do really object to them being in the commit message, I like
this as a backup. It looks like the UX for git would be like:

  git notes --ref checkpatch-ignore append -m "EMAIL_SUBJECT"

Then if you set --notes=checkpatch-ignore in your format-patch command
it comes out like this after the "---":

  Notes (checkpatch-ignore):
      EMAIL_SUBJECT

Downsides?

1. More Perl. But, OK, we have an existence proof that writing Perl is possible.

2. Doesn't seem this can be imported by 'git am'. But, I don't think
that's necessary.

3. That 'git notes' command is a bit unwieldy. But, whatever.

4. With the default Git config, if you rebase your commits you lose the setting.

Point 4 does matter IMO, but it can at least be worked around with:

  git config set notes.rewriteRef "refs/notes/**"

Re: [PATCH 1/2] checkpatch: Add support for Checkpatch-ignore patch footer

[Konstantin Ryabitsev]

On Tue, Jan 14, 2025 at 07:29:14PM +0100, Brendan Jackman wrote:
> > Tweaks aimed at checkpatch are only useful during the code review stage, so
> > once that code is accepted upstream, they become wholly irrelevant. A
> > checkpatch trailer in the permanent commit record serves no purpose, not even
> > a historical one.
> 
> Yeah that's a good argument for them being unnecessary. It's not clear
> why them persisting beyond their useful lifetime would be a problem
> though. Any given reader of a commit message is already very likely to
> see tags they don't care about in that moment, is that something
> people really complain about?

Yes, I expect Linus will reject commits that carry that trailer on the exact
grounds that I brought up. He stated multiple times that a commit message
should only carry trailers that explain the context and the reason for that
change.

-K

Re: [PATCH 1/2] checkpatch: Add support for Checkpatch-ignore patch footer

[Brendan Jackman]

On Tue, 14 Jan 2025 at 20:26, Konstantin Ryabitsev
<konstantin@linuxfoundation.org> wrote:
>
> On Tue, Jan 14, 2025 at 07:29:14PM +0100, Brendan Jackman wrote:
> > is that something people really complain about?
>
> Yes, I expect Linus will reject commits that carry that trailer on the exact
> grounds that I brought up.

Oh OK, Linus is definitely people.

In that case I'll go for the graveyard+git-notes approach. Thanks for the input!

BTW, I also thought of just forgetting the graveyard thing and just
having checkpatch look directly at the Git notes when run in --git
mode. I thought maybe everyone uses --git mode in practice, but I
checked and b4 doesn't seem to. So it's worth making this work for raw
patches too.