Re: [PATCH] Makefile: add pending semantic patches

[Stefan Beller <sbeller@google.com>]

On Thu, Nov 8, 2018 at 8:56 PM Martin Ågren <martin.agren@gmail.com> wrote:
> I haven't followed the original discussion too carefully, so I'll read
> this like someone new to the topic probably would.

Thanks!

> A nit, perhaps, but I was genuinely confused at first. The subject is
> "Makefile: add pending semantic patches", but this patch doesn't add
> any. It adds Makefile-support for such patches though, and it defines
> the entire concept of pending semantic patches. How about "coccicheck:
> introduce 'pending' semantic patches"?
>
> > Add a description and place on how to use coccinelle for large refactorings
> > that happen only once.
>
> A bit confused about "and place". Based on my understanding from reading
> the remainder of this patch, maybe:
>
>   Teach `make coccicheck` to avoid patches named "*.pending.cocci" and
>   handle them separately in a new `make coccicheck-pending` instead.
>   This means that we can separate "critical" patches from "FYI" patches.
>   The former target can continue causing Travis to fail its static
>   analysis job, while the latter can let us keep an eye on ongoing
>   (pending) transitions without them causing too much fallout.
>
>   Document the intended use-cases and processes around these two
>   targets.

Both suggested title and new commit message make sense.

>
> >  This directory provides examples of Coccinelle (http://coccinelle.lip6.fr/)
> >  semantic patches that might be useful to developers.
> > +
> > +There are two types of semantic patches:
> > +
> > + * Using the semantic transformation to check for bad patterns in the code;
> > +   This is what the original target 'make coccicheck' is designed to do and
>
> Is it relevant that this was the "original" target? (Genuine question.)

No. I can omit that part.

>
> > +   it is expected that any resulting patch indicates a regression.
> > +   The patches resulting from 'make coccicheck' are small and infrequent,
> > +   so once they are found, they can be sent to the mailing list as per usual.
> > +
> > +   Example for introducing new patterns:
> > +   67947c34ae (convert "hashcmp() != 0" to "!hasheq()", 2018-08-28)
> > +   b84c783882 (fsck: s/++i > 1/i++/, 2018-10-24)
> > +
> > +   Example of fixes using this approach:
> > +   248f66ed8e (run-command: use strbuf_addstr() for adding a string to
> > +               a strbuf, 2018-03-25)
> > +   f919ffebed (Use MOVE_ARRAY, 2018-01-22)
> > +
> > +   These types of semantic patches are usually part of testing, c.f.
> > +   0860a7641b (travis-ci: fail if Coccinelle static analysis found something
> > +               to transform, 2018-07-23)
>
> Very nicely described, nice with the examples to quickly give a feeling
> about how/when to use this.

Thanks.


>
> > + * Using semantic transformations in large scale refactorings throughout
> > +   the code base.
> > +
> > +   When applying the semantic patch into a real patch, sending it to the
> > +   mailing list in the usual way, such a patch would be expected to have a
> > +   lot of textual and semantic conflicts as such large scale refactorings
> > +   change function signatures that are used widely in the code base.
> > +   A textual conflict would arise if surrounding code near any call of such
> > +   function changes. A semantic conflict arises when other patch series in
> > +   flight introduce calls to such functions.
>
> OK, I'm with you.
>
> > +   So to aid these large scale refactorings, semantic patches can be used,
> > +   using the process as follows:
> > +
> > +   1) Figure out what kind of large scale refactoring we need
> > +      -> This is usually done by another unrelated series.
>
> "This"? The figuring out, or the refactoring? Also, "unrelated"?

The need and type of what kind of large scale refactoring are
usually determined by a patch series, that is not refactoring for the
sake of refactoring, but it wants to achieve a specific goal, unrelated
to large refactorings per se.

The large refactoring is just another tool that a developer can use
to make their original series happen much faster.

So "unrelated" == "not the large scale refactoring, as that may
come as an preparatory series, but to have a preparatory series
it may be good to showcase why we need the preparatory series"

>
> > +   2) Create the sematic patch needed for the large scale refactoring
>
> s/sematic/semantic/

yup.

>
> > +      and store it in contrib/coccinelle/*.pending.cocci
> > +      -> The suffix containing 'pending' is important to differentiate
> > +      this case from the other use case of checking for bad patterns.
>
> Good.
>
> > +   3) Apply the semantic patch only partially, where needed for the patch series
> > +      that motivates the large scale refactoring and then build that series
> > +      on top of it.
> > +      By applying the semantic patch only partially where needed, the series
> > +      is less likely to conflict with other series in flight.
> > +      To make it possible to apply the semantic patch partially, there needs
> > +      to be mechanism for backwards compatibility to keep those places working
> > +      where the semantic patch is not applied. This can be done via a
> > +      wrapper function that has the exact name and signature as the function
> > +      to be changed.
> > +
> > +   4) Send the series as usual, including only the needed parts of the
> > +      large scale refactoring
>
> Trailing period.

ok.

> OK, I think I get it, but I wonder if this might not work equally well
> or better under certain circumstances:
>
>   - introduce new API

The  "new API" is similar enough to the old API and may even be
a superset.

>   - add pending semantic patch
>   - convert quiet areas to use the new API
>
> On the other hand, listing all possible flows might be needlessly
> limiting. I guess it boils down to this:
>
> "Create a pending semantic patch. Make sure the old way of doing things
> still works. Apply the semantic patch to the quieter areas of the
> codebase. If in doubt, convert fewer places in the original series --
> remaining spots can always be converted at a later time."

That's the gist of it. :)

Thanks for the review,
Stefan