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-googOn 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
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?
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
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/**"
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
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.
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.
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".
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)
© 2016 - 2025 Red Hat, Inc.