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
Stefan Beller <sbeller@google.com>
Date
Oct 25, 2018, 19:25 UTC
Message-ID
<CAGZ79kZySpSZC47U2DmP9ia+a-gNw=4MsdkDGGxO0nio9Eo90Q@mail.gmail.com>
In-Reply-To
<20181025015910.GJ30222@szeder.dev>
On Wed, Oct 24, 2018 at 6:59 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
Show 32 quoted lines
>
> On Mon, Oct 22, 2018 at 11:54:06AM -0700, Stefan Beller wrote:
>
> > For the sake of a good history, I would think running 'make coccicheck'
> > and applying the resulting patches would be best as part of the (dirty)
> > merge of any topic that proposes new semantic patches, but that would
> > add load to Junio as it would be an extra step during the merge.
> >
> > One could argue that the step of applying such transformations into
> > the dirty merge is cheaper than resolving merge conflicts that are
> > had when the topic includes the transformation.
>
> Please consider that merge commits' have uglier diffs than regular
> commits, and that merge commits cause additional complications when
> 'git bisect' points the finger at them, both of which are exacerbated
> by additional changes squeezed into evil merges.
>
> > >     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.
> > >
> > >     This was (and still is!) an issue with the hasheq()/oideq() series
> > >     as well: that series was added on 2018-08-28, and the static
> > >     analysis build job is red on 'pu' ever since.  See the follow-up
> > >     patch e43d2dcce1 (more oideq/hasheq conversions, 2018-10-02), and
> > >     one more follow-up will be necessary after the builtin stash topic
> > >     is merged to 'master'.
> >
> > In my understanding this follow up is a feature, as it helps to avoid
> > merge conflicts with other topics in flight.
>
> I don't see how such a follow up patch helps to avoid merge conflicts.

Well, you merge first (the new topic and the cocci patches), and then do the transformation. But that is putting a lot more work on Junio as the way to integrate is not just merge a new topic into the pile of topics (whether it is pu/next/master), but to first perform a merge of the topic and the coccinelle patches, apply the transformation and then merge to the pile, assuming the pile is already transformed (as it happened in "treewide: apply cocci patch" in next/pu).

Show 10 quoted lines
> > as 'make coccicheck' is an integral part of your review?
>
> Erm, right, "review" was not the right word here.  Anyway, as it is,
> 'make coccicheck' is an integral part of our automated tests, not only
> on Travis CI but on the upcoming Azure thing as well.  I just try to
> pay attention to its results and the results of a bunch of my
> additional builds, and complain or even send a fix when something goes
> reproducibly wrong.  This has certainly became more cumbersome with
> the permanently failing static analysis build job in the last couple
> of weeks.

I seem to not pay as much attention as I should to these, mostly because they are unreliable on the aggregate level (a failure there most likely means another topic than the one I am interested broke; except in this case, where we discuss the fallout there via this topic.)

Show 12 quoted lines
> > I like the approach of having separate classes of semantic patches:
> > (a) the regular "we need to keep checking these" as they address
> >     undesirable code patterns, which is what we currently have,
> >     and what 'make coccicheck' would complain about.
> > (b) The pending patches as you propose. However I would
> >     argue that we'd not want to include the transformation into
> >     the same patch as then the patch will have merge conflicts.
>
> Since we have a lot of parallel running topics, merge conflicts are
> basically unavoidable anyway.  If the conflicts from the
> transformation are really that severe, then perhaps the whole series
> should be postponed to a calmer, more suitable time.

Merge conflicts of this kind could be avoided, by running the transformation on both sides before merging (or not running them on both sides, but only after merging).

So maybe for these larger 'the_repository.pending.cocci' patches we could get them into the tree and wait until all (most) of the topics are including that semantic patch in their base, such that application of the patch is easy whether before or after writing a series (as the semantic patch is in its base).

Another short term plan would be renaming the_repository.cocci such that it would break the 'make coccicheck'

> In the case of 'the_repository.cocci', merging its transformations
> into 'pu' resulted in only four conflicts, and I found all four on the
> easy side to resolve.  I don't think it's worth waiting with the
> transformations in this particular case.

I am not worried about the current conflicts, but about those to come in new series.

Show 12 quoted lines
>
> >     Ideally we'd have an automated process/bot that would apply
> >     all pending semantic patches onto master and then checks for
> >     conflicts in HEAD..pu, and only sends off the non-conflicting
> >     diffs as a topic.
>
> New semantic patches didn't pop up all that frequently in the past, so
> I'm not sure it's worth investing in such an automation.  Of course
> they can become more frequent in the future, and in that case we might
> want to reconsider it.  Unfortunately, however, Coccinelle's results
> can't be completely trusted, either because our semantic patches or
> because Coccinelle itself are buggy...

For the first, we could just be become better at reviewing the Cocci patch. ;-) (Well, they are harder to review than usual patches, so this doesn't surprise me. Also not fully understanding the whole tool may have impact on reviewability)

Show 14 quoted lines
>
> >     Then after a couple integration cycles we'd have all pending
> >     changes in, with no conflicts on Junios side.
> >
> > So I think we should add a patch like you post, but we would
> > need to discuss the exact approach how to deal with pending
> > patches. Is it the original dev who should push forward on their
> > own pending patches, or does it become a pooled effort?
>
> Well, it makes sense to me that whoever proposes a change with an
> accompanying new semantic patch should also deal with the necessary
> followups.  However, it doesn't really matter who deals with them, as
> long as somebody deals with them.  I don't think it's much different
> from e.g. sending a followup bugfix to someone else's patch series.

Ok, I plan on resending these patches after I get origin/sb/submodule-recursive-fetch-gets-the-tip going resent.

Stefan
Previous: SZEDER GáborNext: Junio C Hamano
Message 37 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.