git@vger.kernel.org mailing list mirror (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@vger.kernel.org, jonathantanmy@google.com
Subject: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]
Date: Mon, 22 Oct 2018 19:39:35 +0200	[thread overview]
Message-ID: <20181022173935.GG30222@szeder.dev> (raw)
In-Reply-To: <20181016233550.251311-1-sbeller@google.com>

On Tue, Oct 16, 2018 at 04:35:31PM -0700, Stefan Beller wrote:
> the last patch (applying the semantic patches) has been omitted as that
> would produce a lot of merge conflicts. Without that patch, this merges
> cleanly to next.
> 
> As for when to apply the semantic patches, I wondered if we would prefer a dirty merge
> (created via "make coccicheck && git apply contrib/coccinelle/the_repository.cocci.patch")
> of the semantic patches, or if we'd actually trickle in the changes over some time?

>     This series takes another approach as it doesn't change the signature of
>     functions, but introduces new functions that can deal with arbitrary 
>     repositories, keeping the old function signature around using a shallow wrapper.
>     
>     Additionally each patch adds a semantic patch, that would port from the old to
>     the new function. These semantic patches are all applied in the very last patch,
>     but we could omit applying the last patch if it causes too many merge conflicts
>     and trickl in the semantic patches over time when there are no merge conflicts.

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:

  - Applying the transformations from several semantic patches in one
    single patch makes it harder to review it, because we won't know
    which change came from which semantic patch.
    
    For comparison, see the patch series adding hasheq()/oideq(),
    merged in 769af0fd9e (Merge branch 'jk/cocci', 2018-09-17), in
    particular the four "convert <this> to <that>" patches.

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

    This makes it harder to review other patch series.

  - 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
        sha1_to_hex(oid.hash) or strbuf_addf(&sb, "fixed string"), but
        use oid_to_hex(&oid) or strbuf_addstr(&sb, "fixed string")
        instead.  Note that in these cases we don't remove the
        functions sha1_to_hex() or strbuf_addf(), because there are
        good reasons to use them in other scenarios.

        Our semantic patches under 'contrib/coccinelle/' fall into
        this category, and we have 'make coccicheck' and the static
        analysis build job on Travis CI to catch these undesirable
        code patterns preferably early, and to prevent them from
        entering our codebase.

      - To perform one-off code transformations, e.g. to modify a
        function's name and/or signature and convert all its
        callsites; see e.g. commits abef9020e3 (sha1_file: convert
        sha1_object_info* to object_id, 2018-03-12) and b4f5aca40e
        (sha1_file: convert read_sha1_file to struct object_id,
        2018-03-12).

        As far as I understand this patch series falls into this
        category: once the conversion is complete the old functions
        will be removed.  After that there will be no use for these
        semanic patches.

        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.

OK, that was already long.  Now, can we do better?

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?


  -- >8 --

Subject: [PATCH] coccinelle: add support for "pending" semantic patches

(should add a 'contrib/coccinelle/README' to explain what
'.pending.cocci' means)

---
 Makefile | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index d18ab0fe78..67a4441f39 100644
--- a/Makefile
+++ b/Makefile
@@ -2728,9 +2728,11 @@ endif
 	then \
 		echo '    ' SPATCH result: $@; \
 	fi
-coccicheck: $(addsuffix .patch,$(wildcard contrib/coccinelle/*.cocci))
+coccicheck: $(addsuffix .patch,$(filter-out %.pending.cocci,$(wildcard contrib/coccinelle/*.cocci)))
 
-.PHONY: coccicheck
+coccicheck-pending: $(addsuffix .patch,$(wildcard contrib/coccinelle/*.pending.cocci))
+
+.PHONY: coccicheck coccicheck-pending
 
 ### Installation rules
 
-- 
2.19.1.681.g6bd79da3f5




  parent reply	other threads:[~2018-10-22 17:47 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 ` SZEDER Gábor [this message]
2018-10-22 18:54   ` New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base] 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

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=20181022173935.GG30222@szeder.dev \
    --to=szeder.dev@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=jonathantanmy@google.com \
    --cc=peff@peff.net \
    --cc=sbeller@google.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public 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).