git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: "SZEDER Gábor" <szeder.dev@gmail.com>
Cc: Stefan Beller <sbeller@google.com>,
	git@vger.kernel.org, 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 01:39:59 -0400	[thread overview]
Message-ID: <20181025053959.GB11460@sigill.intra.peff.net> (raw)
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:

>   - '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:

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

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

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

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

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

      parent reply	other threads:[~2018-10-25  5:40 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
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 [this message]

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=20181025053959.GB11460@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=jonathantanmy@google.com \
    --cc=sbeller@google.com \
    --cc=szeder.dev@gmail.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).