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

> On Wed, Apr 12 2023, Glen Choo via GitGitGadget wrote:
>
>> +== Authoring and reviewing coccinelle changes
>> +
>> +* When introducing and applying a new .cocci file, both the Git changes and
>> +  .cocci file should be reviewed.
>> +
>> +* Reviewers do not need to be coccinelle experts. To give a Reviewed-By, it is
>> +  enough for the reviewer to get a rough understanding of the proposed rules by
>> +  comparing the .cocci and Git changes, then checking that understanding
>> +  with the author.
>
> Maybe it would be useful here to add something about how you can
> reproduce the application of the coccinelle rule(s).
>
> I sometimes do this on an ad-hoc basis, something like (untested):
>
> 	git checkout HEAD^ -- ':!contrib/coccinelle' '*.[ch]'
> 	make coccicheck
> 	<apply any suggested patches>
> 	git add -A
>
> Then see if I ended up with a no-op, or if there's suggested changes.
>
> With changes that modify both the header & source files this can be
> tricky with the default of SPATCH_USE_O_DEPENDENCIES=Y, but disabling it
> will take care of any potential circular dependency issues. I.e. when
> the header doesn't contain a required construct that we're replacing.

>> +* Conversely, authors should consider that reviewers may not be coccinelle
>> +  experts. The primary aim should be to make .cocci files easy to understand,
>> +  e.g. by adding comments or by using rules that are easier to understand even
>> +  if they are less elegant.
>
> I agree that simple things should be kept simple, but this seems to come
> quite close (or perhaps past the line of) suggesting that we use only
> the simpler features of the language when a more elegant solution would
> be available with something less well-known.
>
> I think we should clarify that that's not the intent. Just as with C,
> shellscript, Perl etc. we should aim for simplicity, but ultimately we
> should expect that we can target the full available language available
> to us.

>> +* .cocci rules should target only the problem it is trying to solve; "collateral
>> +  damage" is not allowed.
>
> I think what you mean here is that you should be able to apply the rule
> and still build the project.

>> +* .cocci files used for refactoring should be temporarily kept in-tree to aid
>> +  the refactoring of out-of-tree code (e.g. in-flight topics). They should be
>> +  removed when enough time has been given for others to refactor their code,
>> +  i.e. ~1 release cycle.
>
> Maybe s/should/can/? E.g. for my recent "index" and "the_repository"
> patches I think they can, but we often keep unused code in-repo for
> longer than that. If e.g. that code stayed in for more than one release
> until someone cared to remove it we'd also be fine.
>
> I also don't know if some long-running forks (e.g. GfW?) would benefit
> from the rules for longer than that...