Re: [PATCH] replay: drop commits that become empty

From: Elijah Newren <newren@gmail.com>
Date: Nov 28, 2025, 08:06 UTC
Message-ID: <CABPp-BEZFPmLnEtnD0WaNbkZ5uE7q5T6uKJQRUvtq+L=C1o9wg@mail.gmail.com>
In-Reply-To: <8a2a1215306452147cc7b803530ab2429bf57f15.1764260150.git.phillip.wood@dunelm.org.uk>

On Thu, Nov 27, 2025 at 8:16 AM Phillip Wood <phillip.wood123@gmail.com> wrote:

Show 8 quoted lines

>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> If the changes in a commit being replayed are already in the branch
> that the commits are being replayed onto then "git replay" creates an
> empty commit. This is confusing because the commit message no longer
> matches the contents of the commit. Drop the commit instead. Commits
> that start off empty are not dropped.

Yeah, I've got a commit in my local branch that does the same thing.

It feels like there should be a paragraph break in here somewhere, but maybe that's just me? Pretty minor either way.

Show 8 quoted lines

> This matches the behavior of
> "git rebase --reapply-cherry-pick --empty=drop" and "git cherry-pick
> --empty-drop". If a branch points to a commit that is dropped it will
> be updated to point to the last commit that was not dropped. This can
> been seen in the new test where "topic1" is updated to point to the
> rebased "C" as "F" is dropped because it is already upstream. While
> this is a breaking change "git replay" is marked as experimental to
> allow improvements like this that change the behavior.

Yep.

Show 6 quoted lines

>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
> Elijah - I'm not really clear why we were setting result->tree before
> calling merge_incore_nonrecursive(), was it just for convenience to
> avoid declaring a local variable or have I missed something?

I don't know the reason. That traces back to a commit with Christian's Co-authored-by, so it may have been either him or me that introduced it. My original work on replay was on a branch that I long ago rebased on top of the version Christian submitted, and the old history is no longer reachable from my local reflog, so I don't have a way to narrow down who of us did it. If it was him, he may be able to answer. If it was me, I've long since forgotten. I think using a temporary, as you've done, is better.

Show 5 quoted lines

> This patch is based on ps/history
>
> I think dropping commits that become empty is the sensible default,
> if it turns out that some users are relying on the current behavior
> we can add an option to retain the empty commits.

I fully agree.

Show 63 quoted lines

> Base-Commit: 4ac8283def34401e50908903b89fa22498bb23a2
> Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Freplay-drop-commits-that-become-empty%2Fv1
> View-Changes-At: https://github.com/phillipwood/git/compare/4ac8283de...8a2a12153
> Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/replay-drop-commits-that-become-empty/v1
>
>  Documentation/git-replay.adoc |  4 +++-
>  replay.c                      | 10 +++++++---
>  t/t3650-replay-basics.sh      | 25 +++++++++++++++++++++++++
>  3 files changed, 35 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/git-replay.adoc b/Documentation/git-replay.adoc
> index dcb26e8a8e8..96a3a557bf3 100644
> --- a/Documentation/git-replay.adoc
> +++ b/Documentation/git-replay.adoc
> @@ -59,7 +59,9 @@ The default mode can be configured via the `replay.refAction` configuration vari
>         be passed, but in `--advance <branch>` mode, they should have
>         a single tip, so that it's clear where <branch> should point
>         to. See "Specifying Ranges" in linkgit:git-rev-parse[1] and the
> -       "Commit Limiting" options below.
> +       "Commit Limiting" options below. Any commits in the range whose
> +       changes are already present in the branch the commits are being
> +       replayed onto will be dropped.
>
>  include::rev-list-options.adoc[]
>
> diff --git a/replay.c b/replay.c
> index 58fdc20140b..7cd7206eee5 100644
> --- a/replay.c
> +++ b/replay.c
> @@ -88,12 +88,12 @@ struct commit *replay_pick_regular_commit(struct repository *repo,
>                                           struct merge_result *result)
>  {
>         struct commit *base, *replayed_base;
> -       struct tree *pickme_tree, *base_tree;
> +       struct tree *pickme_tree, *base_tree, *replayed_base_tree;
>
>         base = pickme->parents->item;
>         replayed_base = mapped_commit(replayed_commits, base, onto);
>
> -       result->tree = repo_get_commit_tree(repo, replayed_base);
> +       replayed_base_tree = repo_get_commit_tree(repo, replayed_base);
>         pickme_tree = repo_get_commit_tree(repo, pickme);
>         base_tree = repo_get_commit_tree(repo, base);
>
> @@ -103,13 +103,17 @@ struct commit *replay_pick_regular_commit(struct repository *repo,
>
>         merge_incore_nonrecursive(merge_opt,
>                                   base_tree,
> -                                 result->tree,
> +                                 replayed_base_tree,
>                                   pickme_tree,
>                                   result);
>
>         free((char*)merge_opt->ancestor);
>         merge_opt->ancestor = NULL;
>         if (!result->clean)
>                 return NULL;
> +       /* Drop commits that become empty */
> +       if (oideq(&replayed_base_tree->object.oid, &result->tree->object.oid) &&
> +           !oideq(&pickme_tree->object.oid, &base_tree->object.oid))
> +               return replayed_base;
>         return replay_create_commit(repo, result->tree, pickme, replayed_base);
>  }

Makes sense; your version is similar but slightly cleaner than my local implementation of the same thing. Plus you have a test, which I hadn't added yet.

Show 18 quoted lines

> diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
> index cf3aacf3551..d73ab16908a 100755
> --- a/t/t3650-replay-basics.sh
> +++ b/t/t3650-replay-basics.sh
> @@ -25,6 +25,8 @@ test_expect_success 'setup' '
>         git switch -c topic3 &&
>         test_commit G &&
>         test_commit H &&
> +       git switch -c empty &&
> +       git commit --allow-empty --only -m empty &&
>         git switch -c topic4 main &&
>         test_commit I &&
>         test_commit J &&
> @@ -106,6 +108,29 @@ test_expect_success 'using replay on bare repo to perform basic cherry-pick' '
>         test_cmp expect result-bare
>  '
>
> +test_expect_success 'commits that become empty are dropped' '

This test is a bit more complicated than normal, and might benefit from a comment or two.

> +       git replay --ref-action=print --advance main topic1^! >result &&
> +       ONTO=$(cut -f 3 -d " " result) &&

You're basically cherry-picking one commit from the middle of A..empty (namely the tip of topic1) onto main, without updating any refs...

> +       git replay --ref-action=print --onto $ONTO \
> +               --branches --ancestry-path=empty ^A >result &&

...and here you replay the range A..empty onto what would have been the new main, but since one of those commits were already cherry-picked, you expect that one to be dropped.

Since "empty" has no descendant commits or branches, the flags
    --branches --ancestry-path=empty ^A
feel like a more complicated way of saying
   --contained A..empty

Show 16 quoted lines

> +       # Write the new value of refs/heads/empty to "new-empty" and
> +       # generate a sed script that annotates the output of
> +       # `git log --format="%H %s"` with the updated branches
> +       SCRIPT="$(sed -e "
> +               /empty/{
> +                       h
> +                       s|^.*empty \([^ ]*\) .*|\1|wnew-empty
> +                       g
> +               }
> +               s|^.*/\([^/ ]*\) \([^ ]*\).*|/^\2/s/\\\$/ (\1)/|
> +               \$s|\$|;s/^[^ ]* //|" result)" &&
> +       git log --format="%H %s" --stdin <new-empty >actual.raw &&
> +       sed -e "$SCRIPT" actual.raw >actual &&
> +       test_write_lines >expect \
> +               "empty (empty)" "H (topic3)" G "C (topic1)" F M L B A &&
> +       test_cmp expect actual

After digging around for a while (my sed-fu is far weaker than yours), this feels like you are going out of your way to avoid changing any branches, but then trying to figure out what the branch changes would have been. Would it be simpler to remove the --ref-action=print flags, check directly what changes were made, and use a test_when_finished to reset the branches back to their starting point at the end? That'd change this test to something like:

test_expect_success 'commits that become empty are dropped' '
    # Save original branches
    git for-each-ref --format="update %(refname) %(objectname)"
refs/heads/ >original-branches &&
    test_when_finished "git update-ref --stdin <original-branches &&
rm original-branches" &&

    # Cherry-pick tip of topic1 ("F"), from the middle of A..empty, to main
    git replay --advance main topic1^! &&

    # Replay all of A..empty onto main (which includes topic1 & thus F
in the middle)
    git replay --onto main --contained A..empty &&

    # Check that "F" was applied first, then "C", and that "F" wasn't
applied twice.  Also, that topic1 now points to "C".
    git log --format="%s%d" L..empty >actual &&
    test_write_lines >expect \
        "empty (empty)" "H (topic3)" G "C (topic1)" F "M (main)" &&
    test_cmp expect actual
'