Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 22, 2018, 22:49 UTC
Message-ID: <xmqqzhv5tx4m.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20181022173935.GG30222@szeder.dev>

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

Show 44 quoted lines

> I don't really like how this or the previous RFC patch series deal
> with semantic patches (or how some past patch series dealt with them,
> for that matter), for various reasons:
> ...
> How about introducing the concept of "pending" semantic patches,
> stored in 'contrib/coccinelle/<name>.pending.cocci' files, modifying
> 'make coccicheck' to skip them, and adding the new 'make
> coccicheck-pending' target to make it convenient to apply them, e.g.
> something like the simple patch at the end.
>
> So the process would go something like this:
>
>   - A new semantic patch should be added as "pending", e.g. to the
>     file 'the_repository.pending.cocci', together with the resulting
>     transformations in the same commit.
>
>     This way neither 'make coccicheck' nor the static analysis build
>     job would complain in the topic branch or in the two integration
>     branches.  And if they do complain, then we would know right away
>     that they complain because of a well-established semantic patch.
>     Yet, anyone interested could run 'make coccicheck-pending' to see
>     where are we heading.
>
>   - The author of the "pending" semanting patch should then keep an
>     eye on already cooking topics: whether any of them contain new
>     code that should be transformed, and how they progress to
>     'master', and sending followup patch(es) with the remaining
>     transformations when applicable.
>     
>     Futhermore, the author should also pay attention to any new topics
>     that branch off after the "pending" semantic patch, and whether
>     any of them introduce code to be transformed, warning their
>     authors as necessary.
>
>   - Finally, after all the dust settled, the dev should follow up with
>     a patch to:
>     
>       - promote the "penging" patch to '<name>.cocci', if its purpose
>         is to avoid undesirable code patterns in the future, or
>     
>       - remove the semantic patch, if it was used in a one-off
>         transformation.
> ...
> Thoughts?

I actually think this round does a far nicer job playing well with other topics than any earlier series. The pain you are observing I think come primarily from my not making the best use of these patches.

Steppng back a bit, I'd imagine in an ideal world where "make coccicheck" can be done instantaneously _and_ the spatch machinery is just as reliable as C compilers. In such a world, I may rename all the *.c files in my tree to *.c.in, make the very first step of the "make all" to run coccicheck and transform *.c.in to *.c before starting the compilation. There is no need to have changes to *.c.in that spatch would fix. Such an idealized set-up has a few nice property.

 - Mechanical conflict resolutions between topics in flight and a
   series that modifies or adds new .cocci rules would greatly be
   reduced.

 - Still, *.c files that are "compiled" from *.c.in file used by
   each build will by definition cocci-clean.

 - Bugs like the one that required 6afedba8 ("object_id.cocci: match
   only expressions of type 'struct object_id'", 2018-10-15) are
   still possible, but some of them may be caught by C compilers
   that inspect the result of spatch compilation from *.c.in to *.c

Now we do not live in that ideal world and there is no separate "turn *.c.in into *.c" step in our build procedure. In such a "real" world, if we had a rule like "each individual commit must pass 'make coccicheck' cleanly", anybody who does such a merge and resolve huge conflics would essentially be wasting time on something that the tool could do, and then the result of the merge would further need to be amended for semantic conflicts (i.e. the other branch may have introduced new instances of old pattern .cocci patches in our branch would want to transform)). By not insisting on cocci-cleanness at each commit level, we can gain (or at least sumulate gaining) some benefit that we would reap in the ideal world, and Stefan's latest series already does so for the former. If we insisted that these patches must be accompanied with the result of the cocci transformations, such a series will have zero chance of landing in 'pu', unless we freeze the world.

What I _could_ do (and what I did do only for one round of pushing out 'pu') is to queue a coccinelle transformation at the tip of integration branches. If "make coccicheck" ran in subsecond, I could even automate it in the script that is used to rebuild 'pu' every day, so that after merging each and every topic, do the "make coccicheck" and apply resulting .cocci.patch files and squash that into the merge commit.

But with the current "make coccicheck" performance, that is not realistic.

I am wondering if it is feasible to do it at the tip of 'pu' (which is rebuilt two to three times a day), 'next' (which is updated once or twice a week) and 'master'.

I find that your "pending" idea may be nicer, as it distributes the load. Whoever wants to change the world order by updating the .cocci rules is primarily responsible for making it happen without breaking the world during the transition. That's more scalable.

But at some point in the ideal future, it may turn out that it is making the process scalable by distributing a task that does not have to be done by any human, not by the integrator or by individual developer---but the new compilation step from *.c.in to *.c should be able to do. We obviously are not there yet, so...