git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] Makefile: add pending semantic patches

From
Stefan Beller <sbeller@google.com>
Date
Nov 9, 2018, 20:50 UTC
Message-ID
<CAGZ79kZRYNjjevWqra2e72rUM3Lu9y1tEL5dDGBLhPKe1o_bGA@mail.gmail.com>
In-Reply-To
<CAN0heSrF7zU=5NGHD9AUcRZgGHoCmrbBwYqJ-6YUM4xg3r+8Rg@mail.gmail.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!
Show 21 quoted lines
> 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.
Show 10 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.)
No. I can omit that part.
Show 20 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.
Thanks.
Show 21 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.
>
> > +   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.
Show 22 quoted lines
>
> > +      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.

Show 10 quoted lines
>   - 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

Previous: Martin ÅgrenNext: Junio C Hamano
Message 6 of 43 in “[PATCHv2 00/24] Bring more repository handles into our code base]”
  1. Stefan BellerOct 30, 2018
  2. 01/24 Makefile: add pending semantic patchesStefan Beller, Oct 30, 2018
  3. Junio C HamanoOct 31, 2018
  4. Makefile: add pending semantic patchesStefan Beller, Nov 8, 2018
  5. Martin ÅgrenNov 9, 2018
  6. Stefan BellerNov 9, 2018
  7. Junio C HamanoNov 9, 2018
  8. Stefan BellerNov 9, 2018
  9. SZEDER GáborNov 13, 2018
  10. Junio C HamanoNov 14, 2018
  11. coccicheck: introduce 'pending' semantic patchesStefan Beller, Nov 10, 2018
  12. Martin ÅgrenNov 10, 2018
  13. Josh SteadmonNov 12, 2018
  14. SZEDER GáborNov 13, 2018
  15. 02/24 sha1_file: allow read_object to read objects in arbitrary repositoriesStefan Beller, Oct 30, 2018
  16. 03/24 packfile: allow has_packed_and_bad to handle arbitrary repositoriesStefan Beller, Oct 30, 2018
  17. 04/24 object-store: allow read_object_file_extended to read from arbitrary repositoriesStefan Beller, Oct 30, 2018
  18. 05/24 object-store: prepare read_object_file to deal with arbitrary repositoriesStefan Beller, Oct 30, 2018
  19. 06/24 object-store: prepare has_{sha1, object}_file[_with_flags] to handle arbitrary repositoriesStefan Beller, Oct 30, 2018
  20. 07/24 object: parse_object to honor its repository argumentStefan Beller, Oct 30, 2018
  21. 08/24 commit: allow parse_commit* to handle arbitrary repositoriesStefan Beller, Oct 30, 2018
  22. 09/24 commit-reach.c: allow paint_down_to_common to handle arbitrary repositoriesStefan Beller, Oct 30, 2018
  23. 10/24 commit-reach.c: allow merge_bases_many to handle arbitrary repositoriesStefan Beller, Oct 30, 2018
  24. 11/24 commit-reach.c: allow remove_redundant to handle arbitrary repositoriesStefan Beller, Oct 30, 2018
  25. 13/24 commit-reach: prepare get_merge_bases to handle arbitrary repositoriesStefan Beller, Oct 30, 2018
  26. 14/24 commit-reach: prepare in_merge_bases[_many] to handle arbitrary repositoriesStefan Beller, Oct 30, 2018
  27. 15/24 commit: prepare get_commit_buffer to handle arbitrary repositoriesStefan Beller, Oct 30, 2018
  28. 12/24 commit-reach.c: allow get_merge_bases_many_0 to handle arbitrary repositoriesStefan Beller, Oct 30, 2018
  29. 16/24 commit: prepare repo_unuse_commit_buffer to handle arbitrary repositoriesStefan Beller, Oct 30, 2018
  30. 17/24 commit: prepare logmsg_reencode to handle arbitrary repositoriesStefan Beller, Oct 30, 2018
  31. 18/24 pretty: prepare format_commit_message to handle arbitrary repositoriesStefan Beller, Oct 30, 2018
  32. 19/24 submodule: use submodule repos for object lookupStefan Beller, Oct 30, 2018
  33. Derrick StoleeNov 2, 2018
  34. Stefan BellerNov 2, 2018
  35. Derrick StoleeNov 2, 2018
  36. 20/24 submodule: don't add submodule as odb for pushStefan Beller, Oct 30, 2018
  37. 21/24 commit-graph: convert remaining function to handle arbitrary repositoriesStefan Beller, Oct 30, 2018
  38. 22/24 commit: make free_commit_buffer and release_commit_memory repository agnosticStefan Beller, Oct 30, 2018
  39. 23/24 path.h: make REPO_GIT_PATH_FUNC repository agnosticStefan Beller, Oct 30, 2018
  40. 24/24 t/helper/test-repository: celebrate independence from the_repositoryStefan Beller, Oct 30, 2018
  41. Junio C HamanoOct 31, 2018
  42. Stefan BellerNov 1, 2018
  43. Junio C HamanoNov 2, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.