Re: [PATCH 01/23] contrib/coccinnelle: add equals-null.cocci

From: Junio C Hamano <gitster@pobox.com>
Date: May 1, 2022, 23:14 UTC
Message-ID: <xmqqbkwg4zi7.fsf@gitster.g>
In-Reply-To: <CA+EOSBnx3-G02=zXGUrRuKPTDPBSYoBY=rERCORe8NtywEOiGg@mail.gmail.com>

Elia Pinto <gitter.spiros@gmail.com> writes:

Show 12 quoted lines

>> What I found curious is that the result of applying these patches to
>> v2.36.0 and running coccicheck reveals that we are not making the
>> codebase clean wrt this new coccinelle rule.
>>
> It is possible, I did not use coccicheck to apply the semantic patch
> (on next)  but i use a my script which I think is slightly more
> efficient but perhaps it is not so correct. Anyway, given the
> discussion that has taken place so far, what do you think is best for
> me to do? Do a reroll (perhaps with only 2 patches in total ) or wait
> for the "right" moment in the future as foreseen by the Documentation
> and dedicate the time to more useful contributions for git? Thank you
> all for the review

Hmph. Even if these patches were created by coccicheck we should sanity check them to make sure we are not applying some stupid and obvious mistakes, but if they were created by an ad-hoc tool, it means we would need to check a lot more careful than a patch that was done with a known tool with a clear rule (which is what running "make coccicheck" with your new rule file would have given us).

To avoid unnecessary conflicts with in-flight topics, ideally, we perhaps could do something along this line:

 * Pick a recent stable point that is an ancestor of all topics in
   flight.  Add the new coccinelle rule file, take "make coccicheck"
   output and create a two-patch series like Philip suggested.  Queue
   the result in a topic branch B.

 * For each topic in flight T, make a trial merge of T into B, and
   examine "make coccicheck" output.  Any new breakages such a test
   finds are new violations the topic T introduces.  Discard the
   result of the trial merge, and add one commit to topic T that
   corrects the violations the topic introduced, and send that fixup
   to the author of the topic for consideration when the topic is
   rerolled (or if the topic is in 'next', acked to be queued on
   top).  Do not fix the violations that is corrected when branch B
   was prepared above.

As I assumed that applying the patches in this series would create the branch B, and then I saw that the tip of 'seen' after merging this topic still needed to have a lot more fixes according to "make coccicheck", I got a (false) impression that there are too many new violations from topics in flight, which was the primary source of my negative reaction against potential code churn. If we try the above exercise, perhaps there may not be too many topics that need fix-up beyond what we fix in the branch B, and if that is the case, I would not be so negative.

Thanks.