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

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

From
Jeff King <peff@peff.net>
Date
Oct 25, 2018, 05:39 UTC
Message-ID
<20181025053959.GB11460@sigill.intra.peff.net>
In-Reply-To
<20181022173935.GG30222@szeder.dev>
On Mon, Oct 22, 2018 at 07:39:35PM +0200, SZEDER Gábor wrote:
> 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:
> [..]

I am a little late to this thread, but it really seems to me that the crux of the issue here:

Show 11 quoted lines
>   - 'make coccicheck' won't run clean (and the static analysis build
>     job on Travis CI will fail) for all commits following adding the
>     new semantic patches but before applying the resulting
>     transformations.
> 
>   - These semantic patches interact badly with 'pu' and 'next',
>     because those integration branches can contain topic branches
>     adding new code that should be transformed by these semanic
>     patches.  Consequently, 'make coccicheck' won't run clean and the
>     static analysis build job will fail until all those topics reach
>     'master', and the remaining transformations are applied on top.

Is that we are considering it a failure for "pu" to have pending coccinelle patches. Are there any coccicheck transformations that aren't just "we prefer to do it like X instead of Y"?

Changes that actually break things should be caught by the compiler (either by their nature, or because we take care to rename or change interfaces when making a semantic change to a function).

I think that's getting at what you're saying here:
Show 7 quoted lines
>   - Is it really necessary to carry these semantic patches in-tree?
>     Let me ellaborate.  There are basically two main use cases for
>     semantic patches:
> 
>       - To avoid undesirable code patterns, e.g. we should not use
> [...]
>       - To perform one-off code transformations, e.g. to modify a

I am mostly thinking of your first type here. And I think it makes sense for people to avoid submitting code that does not pass "make coccicheck" _as it was at the state of their branch_. But for cocci changes that are in flight at the same time as their branch, I do not see any need to strictly adhere to them. The code is "undesirable", not "wrong".

For the second type, I agree that we do not necessarily need to carry them in-tree. Eventually the transformation happens, and nobody would use the "old" way because it doesn't work anymore. Problem solved.

I do not mind carrying them for a while as a convenience to people who will need to fix up their topics in flight (or more likely to the maintainer, who will need to fixup the merge). But we should make sure not to forget to remove them when their usefulness has passed.

Likewise, we should not forget to occasionally run "make coccicheck" on master to make sure people have a clean base to build on. If Junio is able to do that, then great. But other people can also do so and submit patches (to go on their respective topics, or to be a new mass-cleanup topic).

I guess there is some lag there if Junio pushes out a master branch that fails coccicheck, because contributors may build on it before somebody gets around to fixing it.

Show 5 quoted lines
>         Having said that, it's certainly easier to double-check the
>         resulting transformations when one can apply the semantic
>         patches locally, and doing so is easier when the semantic
>         patches are in tree than when they must be copy-pasted from a
>         commit message.

I've wondered if we could have a script that pulls a coccinelle snippet from a commit message and runs it. It may be a hassle to find the right commit, though (you'd start the procedure from "oops, my compile now seems broken; what was the change that I need to apply to adapt?").

Show 10 quoted lines
>   - 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.
OK, makes sense.
Show 10 quoted lines
>   - 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.

This part seems tricky, though. There's a race condition between promoting the patch from pending to not-pending and other topics branching off. And remember that we do not always see other people's branches, which they may work on in private for a long time (though I suppose "when Junio applies it" is one effective cutoff).

Show 8 quoted lines
>   - 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.

I'm not wild about leaving these loose ends that need to be tied up manually. It adds a lot of friction and effort.

-Peff
Previous: Junio C Hamano
Message 46 of 46 in “Bring more repository handles into our code base”
  1. 00/19 Bring more repository handles into our code baseStefan Beller, Oct 16, 2018
  2. 01/19 sha1_file: allow read_object to read objects in arbitrary repositoriesStefan Beller, Oct 16, 2018
  3. 02/19 packfile: allow has_packed_and_bad to handle arbitrary repositoriesStefan Beller, Oct 16, 2018
  4. 03/19 object-store: allow read_object_file_extended to read from arbitrary repositoriesStefan Beller, Oct 16, 2018
  5. 04/19 object-store: prepare read_object_file to deal with arbitrary repositoriesStefan Beller, Oct 16, 2018
  6. 05/19 object-store: prepare has_{sha1, object}_file[_with_flags] to handle arbitrary repositoriesStefan Beller, Oct 16, 2018
  7. 06/19 object: parse_object to honor its repository argumentStefan Beller, Oct 16, 2018
  8. 07/19 commit: allow parse_commit* to handle arbitrary repositoriesStefan Beller, Oct 16, 2018
  9. 08/19 commit-reach.c: allow paint_down_to_common to handle arbitrary repositoriesStefan Beller, Oct 16, 2018
  10. 09/19 commit-reach.c: allow merge_bases_many to handle arbitrary repositoriesStefan Beller, Oct 16, 2018
  11. 10/19 commit-reach.c: allow remove_redundant to handle arbitrary repositoriesStefan Beller, Oct 16, 2018
  12. 11/19 commit-reach.c: allow get_merge_bases_many_0 to handle arbitrary repositoriesStefan Beller, Oct 16, 2018
  13. 12/19 commit-reach: prepare get_merge_bases to handle arbitrary repositoriesStefan Beller, Oct 16, 2018
  14. 13/19 commit-reach: prepare in_merge_bases[_many] to handle arbitrary repositoriesStefan Beller, Oct 16, 2018
  15. 14/19 commit: prepare get_commit_buffer to handle arbitrary repositoriesStefan Beller, Oct 16, 2018
  16. 15/19 commit: prepare repo_unuse_commit_buffer to handle arbitrary repositoriesStefan Beller, Oct 16, 2018
  17. 16/19 commit: prepare logmsg_reencode to handle arbitrary repositoriesStefan Beller, Oct 16, 2018
  18. 17/19 pretty: prepare format_commit_message to handle arbitrary repositoriesStefan Beller, Oct 16, 2018
  19. 18/19 submodule: use submodule repos for object lookupStefan Beller, Oct 16, 2018
  20. Jonathan TanOct 19, 2018
  21. SZEDER GáborOct 25, 2018
  22. Derrick StoleeOct 31, 2018
  23. Stefan BellerNov 1, 2018
  24. 19/19 submodule: don't add submodule as odb for pushStefan Beller, Oct 16, 2018
  25. Jonathan TanOct 19, 2018
  26. Derrick StoleeOct 17, 2018
  27. Stefan BellerOct 17, 2018
  28. 0/2 Bring the_repository into cmd_fooStefan Beller, Oct 18, 2018
  29. 1/2 repository: have get_the_repository() to remove the_repository dependencyStefan Beller, Oct 18, 2018
  30. 2/2 builtin/merge-base.c: do not rely on the_repository any moreStefan Beller, Oct 18, 2018
  31. Jonathan TanOct 18, 2018
  32. Stefan BellerOct 18, 2018
  33. Junio C HamanoOct 19, 2018
  34. New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]SZEDER Gábor, Oct 22, 2018
  35. Stefan BellerOct 22, 2018
  36. SZEDER GáborOct 25, 2018
  37. Stefan BellerOct 25, 2018
  38. Junio C HamanoOct 22, 2018
  39. Stefan BellerOct 23, 2018
  40. Junio C HamanoOct 23, 2018
  41. Junio C HamanoOct 23, 2018
  42. Carlo ArenasOct 23, 2018
  43. Junio C HamanoOct 23, 2018
  44. Stefan BellerOct 23, 2018
  45. Junio C HamanoOct 24, 2018
  46. Jeff KingOct 25, 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.