Re: [PATCH] Makefile: add pending semantic patches

From: "Martin Ågren" <martin.agren@gmail.com>
To: Stefan Beller <sbeller@google.com>
Cc: "Git Mailing List" <git@vger.kernel.org>,
	"Jonathan Tan" <jonathantanmy@google.com>,
	"SZEDER Gábor" <szeder.dev@gmail.com>
Subject: Re: [PATCH] Makefile: add pending semantic patches
Date: Fri, 9 Nov 2018 05:55:53 +0100	[thread overview]
Message-ID: <CAN0heSrF7zU=5NGHD9AUcRZgGHoCmrbBwYqJ-6YUM4xg3r+8Rg@mail.gmail.com> (raw)
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.

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

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

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

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

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