Re: How do we review changes made with coccinelle?

From: Taylor Blau <me@ttaylorr.com>
Date: Mar 31, 2023, 23:49 UTC
Message-ID: <ZCdxcKr2mQ5cBQ8u@nand.local>
In-Reply-To: <xmqqtty2hx30.fsf@gitster.g>

On Thu, Mar 30, 2023 at 12:13:07PM -0700, Junio C Hamano wrote:

Show 9 quoted lines

> Glen Choo <chooglen@google.com> writes:
>
> > - Is it okay to give Reviewed-By on the basis of _just_ the in-tree
> >   changes and ignore the .cocci patch?
>
> If they were made in separate steps, sure.  If not, not really.  But
> we can still say "I've checked the changes the author made to the
> code and they looked good."  But we haven't reviewed the patch in
> its entirety in such a case to give a Reviewed-by, I would thihk.

I think that while none of us would probably call ourselves "Coccinelle experts", we are probably reasonably capable of reviewing *.cocci files and their impact on the tree.

What I meant when we were talking about this off-list was that I don't consider myself an expert at writing idiomatic Coccinelle rules. But I feel competent enough that I could review Ævar's patches by roughly grokking the *.cocci changes, and then checking that the resulting tree state matched my understanding of those changes.

> > - Do we care about new patches slowing down coccicheck?

I was the one who asked this question off-list, and I did so in a leading way that implied that the answer was "no".

> Surely.

But I agree with Junio that we *do* care about slowing down the performance of 'make coccicheck'. When I originally asked, I was under the (false) impression that we didn't run 'make coccicheck' in CI. But we do (see ci/run-static-analysis.sh), so we do care about the performance there since we don't want to unnecessarily slow down CI.

Thanks, Taylor