Re: [PATCH 2/2] cocci: codify authoring and reviewing practices

From: Glen Choo <chooglen@google.com>
To: "SZEDER Gábor" <szeder.dev@gmail.com>,
	"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, "Taylor Blau" <me@ttaylorr.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Elijah Newren" <newren@gmail.com>
Subject: Re: [PATCH 2/2] cocci: codify authoring and reviewing practices
Date: Wed, 19 Apr 2023 12:29:32 -0700	[thread overview]
Message-ID: <kl6lzg731xib.fsf@chooglen-macbookpro.roam.corp.google.com> (raw)
In-Reply-To: <20230416074212.GB3271@szeder.dev>

SZEDER Gábor <szeder.dev@gmail.com> writes:

>> +* .cocci rules should target only the problem it is trying to solve; "collateral
>> +  damage" is not allowed.
>> +
>> +* .cocci files used for refactoring should be temporarily kept in-tree to aid
>
> How should such semantic patches be kept in-tree?
> As .pending.cocci?  Then I think it would be better to point this out
> here.  Or as a "regular" semantic patch?  Then I'm not sure I agree
> with this recommendation, but perhaps a commit message explaining the
> reasoning behind this would help me make up my mind :)

I don't feel strongly about this, but I was envisioning keeping them as
a "regular" patch, e.g. what Ævar proposed in:

  https://lore.kernel.org/git/230326.86ileow1fu.gmgdl@evledraar.gmail.com/

In theory, this means that a long running fork (that didn't get updated
during the initial refactor) can run coccicheck, notice the failure, and
then automatically fix themselves with the included semantic patch. In
practice, I don't know how many forks run coccicheck, or whether these
refactors are just easy enough to do by hand.

For refactors, I suspect that the impact on the 'make coccicheck'
runtime will be low, since we're typically targeting just a few tokens
and cocci can skip whatever files don't have those tokens, so keeping it
as a "regular" patch might be okay.

> It might also be worth mentioning that before submitting a new
> semantic patch developers should consider its cost-benefit ratio, in
> particular its effect on the runtime of 'make coccicheck',

Makes sense, though I'm not sure what practical advice to give in order
to evaluate the impact on runtime (besides just running it themselves).

> in the hope
> that we can avoid another 'unused.cocci' fiasco.

Maybe this is a good starting point for discussing cost-benefit
analysis. I'm not familiar with this fiasco, though. Was an early
version of 'unused.cocci' too broad, resulting in a massive hit to
runtime?