[PATCH v2 0/2] checkpatch: Add support for checkpatch-ignore note

[PATCH v2 0/2] checkpatch: Add support for checkpatch-ignore note

[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 patch "graveyard" (after the
"---" that a patch might produce a certain checkpatch error, and that
this is an expected false positive.

Note, for Git users this will require some configuration changes to
adopt (see documentation patch), and not all tools that wrap Checkpatch
will automatically be able to take advantage. Changes are required for
`b4 prep --check` to work [0], I'll send that separately if this patch
is accepted.

[0] https://github.com/bjackman/b4/tree/checkpatch-ignore

Signed-off-by: Brendan Jackman <jackmanb@google.com>
---
Changes in v2:
- Switched to the "graveyard" instead of the actual commit message.
- Link to v1: https://lore.kernel.org/r/20250113-checkpatch-ignore-v1-0-63a7a740f568@google.com

---
Brendan Jackman (2):
      checkpatch: Add support for checkpatch-ignore notes
      docs: checkpatch: Document checkpatch-ignore feature

 Documentation/dev-tools/checkpatch.rst | 46 ++++++++++++++++++++++++++++++++++
 scripts/checkpatch.pl                  | 34 +++++++++++++++++++++++--
 2 files changed, 78 insertions(+), 2 deletions(-)
---
base-commit: 619f0b6fad524f08d493a98d55bac9ab8895e3a6
change-id: 20250113-checkpatch-ignore-1096914844eb

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

[PATCH v2 1/2] checkpatch: Add support for checkpatch-ignore notes

[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 mechanism to record alongside the patch that it might
produce certain checkpatch errors, and that these are expected false
positives. There are two aspects to this mechanism:

1. If a block like:

   Notes (checkpatch-ignore):
     FOO,BAR
     BAZ

   Is found before the diff in the patch content, FOO, BAR and BAZ error
   types are ignored while processing this file.

   Its expected that users put this in the "graveyard" i.e. the region
   between the --- and the beginning of the diff.

2. --notes=checkpatch-ignore is added to the `git format-patch`
   command that checkpatch.pl uses in --git mode, so that if the commit
   being inspected has a note [1] under the checkpatch-ignore ref, it
   will be formatted into a block like the one above.

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).

Because the main loop in process() begins to emit errors before it has
necessarily processed the checkpatch-ignore block, this parsing is done
separately in its own loop.

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

[1] https://git-scm.com/docs/git-notes

Signed-off-by: Brendan Jackman <jackmanb@google.com>
---

Notes (checkpatch-ignore):
    EMAIL_SUBJECT

 scripts/checkpatch.pl | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 9eed3683ad76caffbbb2418e5dbea7551d374406..ce6914a845ec3f936ad656fa123f58aa85ce4b2f 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";
@@ -1306,7 +1309,7 @@ for my $filename (@ARGV) {
 	my $oldfile = $file;
 	$file = 1 if ($is_git_file);
 	if ($git) {
-		open($FILE, '-|', "git format-patch -M --stdout -1 $filename") ||
+		open($FILE, '-|', "git format-patch --notes=checkpatch-ignore -M --stdout -1 $filename") ||
 			die "$P: $filename: git format-patch failed - $!\n";
 	} elsif ($file) {
 		open($FILE, '-|', "diff -u /dev/null $filename") ||
@@ -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 {
@@ -2624,6 +2627,29 @@ sub exclude_global_initialisers {
 		$realfile =~ m@/bpf/.*\.bpf\.c$@;
 }
 
+# Parse the "Notes (checkpatch-ignore):" block in the region before the diff,
+# and set file_ignore_type accordingly.
+sub parse_checkpatch_ignore {
+	my $linesRef = shift;
+	my $in_checkpatch_ignore = 0;
+
+	foreach my $line (@$linesRef) {
+		# have we reached the actual diff?
+		if ($line =~ /^diff --git.*?(\s+)$/ || $line =~ /^\+\+\+\s+(\s+)/) {
+			last;
+		}
+
+		if ($in_checkpatch_ignore) {
+			if ($line =~ /^\s*$/) {
+				last;
+			}
+			hash_save_array_words(\%file_ignore_type, [$line]);
+		} elsif ($line =~ /^Notes \(checkpatch-ignore\):\s*/) {
+			$in_checkpatch_ignore = 1;
+		}
+	}
+}
+
 sub process {
 	my $filename = shift;
 
@@ -2701,6 +2727,8 @@ sub process {
 
 	my $checklicenseline = 1;
 
+	%file_ignore_type = ();
+
 	sanitise_line_reset();
 	my $line;
 	foreach my $rawline (@rawlines) {
@@ -2780,6 +2808,8 @@ sub process {
 		}
 	}
 
+	parse_checkpatch_ignore(\@lines);
+
 	$prefix = '';
 
 	$realcnt = 0;

-- 
2.48.0.rc2.279.g1de40edade-goog

[PATCH v2 2/2] docs: checkpatch: Document checkpatch-ignore feature

[Brendan Jackman]

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

It requires some rather obscure Git features to take advantage of
this, so provide some examples of how to do that.

Signed-off-by: Brendan Jackman <jackmanb@google.com>
---

Notes (checkpatch-ignore):
    EMAIL_SUBJECT

 Documentation/dev-tools/checkpatch.rst | 46 ++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
index abb3ff6820766ee0c29112b256bcc44ce41fffba..6215b24b25b36709c815cf08de33f1609c80c0c7 100644
--- a/Documentation/dev-tools/checkpatch.rst
+++ b/Documentation/dev-tools/checkpatch.rst
@@ -12,6 +12,49 @@ 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.
 
+Inoring violations
+==================
+
+As well as the --ignore flag documented below, violation types can be ignored
+for a specific patch by including a block after the "---" in the following
+format::
+
+  Notes(checkpatch-ignore):
+    TYPE_1,TYPE_2
+    TYPE_3
+
+If using Git, you can store that information alongside your commit using
+`notes <https://git-scm.com/docs/git-notes>`_. To set this up:
+
+1. Configure git to include the `checkpatch-ignore` notes ref in formatted
+   patches::
+
+     git config set format.notes checkpatch-ignore
+
+   If you use checkpatch in `--git` mode, this isn't necessary, it will include
+   the `checkpatch-ignore` note regardless.
+
+2. Configure git to persist notes across amends and rebases::
+
+     git config set notes.rewriteRef "refs/notes/checkpatch-ignore"
+
+   (To enable this behaviour for _all_ notes, set `refs/notes/**` instead).
+
+   Also ensure that `notes.rewrite.rebase` and `notes.rewrite.amend` have not
+   been set to `false`.
+
+3. Now, to set the note on the HEAD commit, use a command like::
+
+     git notes --ref checkpatch-ignore add -m "TYPE_1,TYPE_2"
+
+   Beware that blank lines terminate the `checkpatch-ignore` block, so if you
+   use `git notes append` to ignore additional types, you'll need to also set
+   `--no-separator`::
+
+     git notes --ref checkpatch-ignore append -m "TYPE_3" --no-separator
+
+To see the names of the error type in checkpatch output, set the `--show-types`
+option.
 
 Options
 =======
@@ -114,6 +157,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.48.0.rc2.279.g1de40edade-goog

Re: [PATCH v2 2/2] docs: checkpatch: Document checkpatch-ignore feature

[Brendan Jackman]

On Wed, 15 Jan 2025 at 16:33, Brendan Jackman <jackmanb@google.com> wrote:
> +1. Configure git to include the `checkpatch-ignore` notes ref in formatted

Argh, make htmldocs just finished (it takes an extremely long time for
me...) and I realised I sent this without checking the docs output.
Turns out backticks aren't the right syntax for .rst.

Will wait for feedback on the actual thing before fixing this.

Re: [PATCH v2 0/2] checkpatch: Add support for checkpatch-ignore note

[Brendan Jackman]

On Wed, 15 Jan 2025 at 16:33, Brendan Jackman <jackmanb@google.com> wrote:
> Changes in v2:
> - Switched to the "graveyard" instead of the actual commit message.
> - Link to v1: https://lore.kernel.org/r/20250113-checkpatch-ignore-v1-0-63a7a740f568@google.com

Hi Joe & Andy, any thoughts on this?

Re: [PATCH v2 0/2] checkpatch: Add support for checkpatch-ignore note

[Jeff Johnson]

On 1/15/2025 7:33 AM, 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 patch "graveyard" (after the
> "---" that a patch might produce a certain checkpatch error, and that
> this is an expected false positive.
> 
> Note, for Git users this will require some configuration changes to
> adopt (see documentation patch), and not all tools that wrap Checkpatch
> will automatically be able to take advantage. Changes are required for
> `b4 prep --check` to work [0], I'll send that separately if this patch
> is accepted.
> 
> [0] https://github.com/bjackman/b4/tree/checkpatch-ignore

While this addresses one issue with checkpatch, it doesn't solve what I
consider to be a bigger issue, namely to have a way for checkpatch to ignore
false positives (or deliberate use of non-compliant code) when run with the -f
flag.

I don't know how many times I've seen new kernel contributors try to "fix"
checkpatch issues as their first commit, and contribute broken code in the
process. This is especially true when trying to "fix" macros.

So IMO what would be more useful is to have some way to document these
overrides in the tree so that they could be honored both when processing
patches as well as when processing files.

Note that thanks to Kalle's work, the wireless/ath drivers have their own way
of doing this, but of course that only works if you run the scripts.
checkpatch run normally would still flag the issues.

more surgical:
<https://github.com/qca/qca-swiss-army-knife/blob/master/tools/scripts/ath12k/ath12k-check>

less surgical:
<https://github.com/qca/qca-swiss-army-knife/blob/master/tools/scripts/ath11k/ath11k-check>
<https://github.com/qca/qca-swiss-army-knife/blob/master/tools/scripts/ath10k/ath10k-check>

/jeff

Re: [PATCH v2 0/2] checkpatch: Add support for checkpatch-ignore note

[Brendan Jackman]

On Mon, 24 Feb 2025 at 20:09, Jeff Johnson
<jeff.johnson@oss.qualcomm.com> wrote:
>
> On 1/15/2025 7:33 AM, 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 patch "graveyard" (after the
> > "---" that a patch might produce a certain checkpatch error, and that
> > this is an expected false positive.
> >
> > Note, for Git users this will require some configuration changes to
> > adopt (see documentation patch), and not all tools that wrap Checkpatch
> > will automatically be able to take advantage. Changes are required for
> > `b4 prep --check` to work [0], I'll send that separately if this patch
> > is accepted.
> >
> > [0] https://github.com/bjackman/b4/tree/checkpatch-ignore
>
> While this addresses one issue with checkpatch, it doesn't solve what I
> consider to be a bigger issue, namely to have a way for checkpatch to ignore
> false positives (or deliberate use of non-compliant code) when run with the -f
> flag.
>
> I don't know how many times I've seen new kernel contributors try to "fix"
> checkpatch issues as their first commit, and contribute broken code in the
> process. This is especially true when trying to "fix" macros.
>
> So IMO what would be more useful is to have some way to document these
> overrides in the tree so that they could be honored both when processing
> patches as well as when processing files.
>
> Note that thanks to Kalle's work, the wireless/ath drivers have their own way
> of doing this, but of course that only works if you run the scripts.
> checkpatch run normally would still flag the issues.
>
> more surgical:
> <https://github.com/qca/qca-swiss-army-knife/blob/master/tools/scripts/ath12k/ath12k-check>
>
> less surgical:
> <https://github.com/qca/qca-swiss-army-knife/blob/master/tools/scripts/ath11k/ath11k-check>
> <https://github.com/qca/qca-swiss-army-knife/blob/master/tools/scripts/ath10k/ath10k-check>

Yeah I think the best solution for that would be something like a
.checkpatch-ignore file in the spirit of .gitignore.

Maybe other tools have solutions for that that checkpatch could copy.
The only one I know of is pylint which solves it with code comments.
That makes a real visual mess of the code in my experience. And based
on Konstantin's comments on v1 of this patch, comments _definitely_
wouldn't fly with the kernel community! So I think it would have to be
an out-of-band config, and if that comes at the expense of granularity
then so be it.

(I would support the inline-config approach for a very high-precision
linter, like Rust and Go have, but Checkpatch Dot P.L. is never gonna
be one of those).

Anyway, solving the -f case shouldn't be required for merging this feature IMO.