From: Phillip Wood <phillip.wood123@gmail.com>
Date: Thu, 04 Dec 2025 14:06:51 GMT
Subject: Re: [PATCH] replay: drop commits that become empty
Message-ID: <10f9afd8-6ac2-4e17-979f-2222bd0a2fda@gmail.com>
In-Reply-To: <CABPp-BEZFPmLnEtnD0WaNbkZ5uE7q5T6uKJQRUvtq+L=C1o9wg@mail.gmail.com>

On 28/11/2025 08:06, Elijah Newren wrote:
> On Thu, Nov 27, 2025 at 8:16 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> 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.

Yes it could do with a paragraph break, I'll add one

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

Thanks, I was worried I might have missed some subtlety and 
inadvertently broken a corner case.

>> +       # 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:

I used --ref-action=print to match the existing tests, but it would be 
much simpler to drop it. Your suggestion below looks good.

Thanks

Phillip


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