Re: [PATCH] Makefile: add pending semantic patches

From: Martin Ågren <martin.agren@gmail.com>
Date: Nov 9, 2018, 04:55 UTC
Message-ID: <CAN0heSrF7zU=5NGHD9AUcRZgGHoCmrbBwYqJ-6YUM4xg3r+8Rg@mail.gmail.com>
In-Reply-To: <20181108205255.193402-1-sbeller@google.com>

On Thu, 8 Nov 2018 at 21:53, Stefan Beller <sbeller@google.com> wrote:

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

I haven't followed the original discussion too carefully, so I'll read this like someone new to the topic probably would.

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.

Show 7 quoted lines

>  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.)

Show 16 quoted lines

> +   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.

Show 10 quoted lines

> + * 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.

Show 5 quoted lines

> +   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"?

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

s/sematic/semantic/

> +      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.

Show 13 quoted lines

> +   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, I think I get it, but I wonder if this might not work equally well or better under certain circumstances:

  - introduce new API
  - 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."

Martin