git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: "SZEDER Gábor" <szeder.dev@gmail.com>
To: Stefan Beller <sbeller@google.com>
Cc: Jeff King <peff@peff.net>, git <git@vger.kernel.org>,
	Jonathan Tan <jonathantanmy@google.com>
Subject: Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]
Date: Thu, 25 Oct 2018 03:59:10 +0200	[thread overview]
Message-ID: <20181025015910.GJ30222@szeder.dev> (raw)
In-Reply-To: <CAGZ79kboyakR=dARH60ZJmyN=kT+-X0SSDjgz3aswpkUd+bZmg@mail.gmail.com>

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.

There were topics that branched off before the introduction of oideq()
into 'master', therefore they couldn't make use of this new function
until they were merged to 'master' as well, so they added their own
!oidcmp() calls.  That follow up patch was necessary to transform
these new !oidcmp() calls after those topics reached 'master'.  Merge
conflicts had nothing to do with it.

So this follow up patch is not a feature, but rather an inherent
consequence of the project's branching model, with lots of parallel
running topics branching off at different points and progressing at
different speeds.

> >     This makes it harder to review other patch series.
> 
> 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.

> > How about introducing the concept of "pending" semantic patches,
> > stored in 'contrib/coccinelle/<name>.pending.cocci' files, modifying
> > 'make coccicheck' to skip them, and adding the new 'make
> > coccicheck-pending' target to make it convenient to apply them, e.g.
> > something like the simple patch at the end.
> >
> > So the process would go something like this:
> >
> >   - 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.
> >
> >   - 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.
> >
> >   - 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.
> >
> > Thoughts?
> 
> 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.

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.

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

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



  reply	other threads:[~2018-10-25  1:59 UTC|newest]

Thread overview: 46+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-16 23:35 [PATCH 00/19] Bring more repository handles into our code base Stefan Beller
2018-10-16 23:35 ` [PATCH 01/19] sha1_file: allow read_object to read objects in arbitrary repositories Stefan Beller
2018-10-16 23:35 ` [PATCH 02/19] packfile: allow has_packed_and_bad to handle " Stefan Beller
2018-10-16 23:35 ` [PATCH 03/19] object-store: allow read_object_file_extended to read from " Stefan Beller
2018-10-16 23:35 ` [PATCH 04/19] object-store: prepare read_object_file to deal with " Stefan Beller
2018-10-16 23:35 ` [PATCH 05/19] object-store: prepare has_{sha1, object}_file[_with_flags] to handle " Stefan Beller
2018-10-16 23:35 ` [PATCH 06/19] object: parse_object to honor its repository argument Stefan Beller
2018-10-16 23:35 ` [PATCH 07/19] commit: allow parse_commit* to handle arbitrary repositories Stefan Beller
2018-10-16 23:35 ` [PATCH 08/19] commit-reach.c: allow paint_down_to_common " Stefan Beller
2018-10-16 23:35 ` [PATCH 09/19] commit-reach.c: allow merge_bases_many " Stefan Beller
2018-10-16 23:35 ` [PATCH 10/19] commit-reach.c: allow remove_redundant " Stefan Beller
2018-10-16 23:35 ` [PATCH 11/19] commit-reach.c: allow get_merge_bases_many_0 " Stefan Beller
2018-10-16 23:35 ` [PATCH 12/19] commit-reach: prepare get_merge_bases " Stefan Beller
2018-10-16 23:35 ` [PATCH 13/19] commit-reach: prepare in_merge_bases[_many] " Stefan Beller
2018-10-16 23:35 ` [PATCH 14/19] commit: prepare get_commit_buffer " Stefan Beller
2018-10-16 23:35 ` [PATCH 15/19] commit: prepare repo_unuse_commit_buffer " Stefan Beller
2018-10-16 23:35 ` [PATCH 16/19] commit: prepare logmsg_reencode " Stefan Beller
2018-10-16 23:35 ` [PATCH 17/19] pretty: prepare format_commit_message " Stefan Beller
2018-10-16 23:35 ` [PATCH 18/19] submodule: use submodule repos for object lookup Stefan Beller
2018-10-19 20:37   ` Jonathan Tan
2018-10-25  9:14   ` SZEDER Gábor
2018-10-31 13:38   ` Derrick Stolee
2018-11-01 19:13     ` Stefan Beller
2018-10-16 23:35 ` [PATCH 19/19] submodule: don't add submodule as odb for push Stefan Beller
2018-10-19 20:39   ` Jonathan Tan
2018-10-17 12:41 ` [PATCH 00/19] Bring more repository handles into our code base Derrick Stolee
2018-10-17 17:53   ` Stefan Beller
2018-10-18 18:37     ` [RFC PATCH 0/2] Bring the_repository into cmd_foo Stefan Beller
2018-10-18 18:37       ` [RFC PATCH 1/2] repository: have get_the_repository() to remove the_repository dependency Stefan Beller
2018-10-18 18:37       ` [RFC PATCH 2/2 (BREAKS BUILD)] builtin/merge-base.c: do not rely on the_repository any more Stefan Beller
2018-10-18 21:01       ` [RFC PATCH 0/2] Bring the_repository into cmd_foo Jonathan Tan
2018-10-18 23:23         ` Stefan Beller
2018-10-19  7:23 ` [PATCH 00/19] Bring more repository handles into our code base Junio C Hamano
2018-10-22 17:39 ` New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base] SZEDER Gábor
2018-10-22 18:54   ` Stefan Beller
2018-10-25  1:59     ` SZEDER Gábor [this message]
2018-10-25 19:25       ` Stefan Beller
2018-10-22 22:49   ` Junio C Hamano
2018-10-23  0:26     ` Stefan Beller
2018-10-23  4:24       ` Junio C Hamano
2018-10-23  9:38     ` Junio C Hamano
2018-10-23 10:15       ` Carlo Arenas
2018-10-23 10:21         ` Junio C Hamano
2018-10-23 17:30       ` Stefan Beller
2018-10-24  1:22         ` Junio C Hamano
2018-10-25  5:39   ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181025015910.GJ30222@szeder.dev \
    --to=szeder.dev@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=jonathantanmy@google.com \
    --cc=peff@peff.net \
    --cc=sbeller@google.com \
    --subject='Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).