Re: How do we review changes made with coccinelle?

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 30, 2023, 19:13 UTC
Message-ID: <xmqqtty2hx30.fsf@gitster.g>
In-Reply-To: <kl6l7cuycd3n.fsf@chooglen-macbookpro.roam.corp.google.com>

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.

> - What do we do with .cocci after they've been applied?

When we keep .cocci rules in tree, "make coccicheck" would complain on any new code that matches the preimage pattern of these rules and adjust them. Your use of memcpy() may be rewritten to COPY_ARRAY() when appropriate. At least that is the theory---an overly wide or ad-hoc rule that depends too much on heuristic may misconvert future code, which needs to be caught by reviewers when .cocci files are added to the tree.

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

Surely.

We may want to cull rules from time to time. For example, a rule that moves callers of an older API function to use a newer API function has to be kept while the older API function still exists in the tree to help topics that are still in flight, but eventually everybody stops using it and the implementation of the older API function gets removed. We should make sure we remove the rule that is now stale.