Message-ID: <CA+i-1C30dTL4H2ELNzvYV_yOc+WT=bVKW7R3KQe6=XjRX_jzfw@mail.gmail.com>
Date: Tue, 25 Feb 2025 12:33:27 +0100
From: Brendan Jackman <jackmanb@...gle.com>
To: Jeff Johnson <jeff.johnson@....qualcomm.com>
Cc: Andy Whitcroft <apw@...onical.com>, Joe Perches <joe@...ches.com>, 
	Dwaipayan Ray <dwaipayanray1@...il.com>, Lukas Bulwahn <lukas.bulwahn@...il.com>, 
	Jonathan Corbet <corbet@....net>, Konstantin Ryabitsev <konstantin@...uxfoundation.org>, 
	linux-kernel@...r.kernel.org, workflows@...r.kernel.org, 
	linux-doc@...r.kernel.org
Subject: Re: [PATCH v2 0/2] checkpatch: Add support for checkpatch-ignore note

On Mon, 24 Feb 2025 at 20:09, Jeff Johnson
<jeff.johnson@....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.

Powered by blists - more mailing lists