Re: [PATCH v2 3/3] replay: offer an option to linearize the commit topology

[Toon Claes <toon@iotcl.com>]

Elijah Newren <newren@gmail.com> writes:

> Hi,
>
> On Wed, Jun 10, 2026 at 7:51 AM Toon Claes <toon@iotcl.com> wrote:
>>
>> From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
>>
>> One of the stated goals of git-replay(1) is to allow implementing the
>> git-rebase(1) functionality on the server side.
>>
>> The default mode of git-rebase(1) is to act as if `--no-rebase-merges`
>> was given. This mode drops merge commits instead of replaying them, and
>> linearizes the commit history into a sequence of the
>> regular (single-parent) commits.
>>
>> Add option `--linearize` to git-replay(1) to do the same.
>
> I think this version is nicer overall than the one from my
> replay-upstream branch; sorry for repeatedly getting distracted from
> that, but this does look nice.

Dscho gets most of the credit here. And don't worry about being
distracted, we know how things go around here. I appreciate the review!

>
> A few small comments:
>
>> Co-authored-by: Toon Claes <toon@iotcl.com>
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> Signed-off-by: Toon Claes <toon@iotcl.com>
>> ---
>>  Documentation/git-replay.adoc |  5 +++++
>>  builtin/replay.c              |  4 ++++
>>  replay.c                      | 30 +++++++++++++++++++++++-------
>>  replay.h                      |  5 +++++
>>  t/t3650-replay-basics.sh      | 26 ++++++++++++++++++++++++++
>>  5 files changed, 63 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/git-replay.adoc b/Documentation/git-replay.adoc
>> index a32f72aead..41c96c7061 100644
>> --- a/Documentation/git-replay.adoc
>> +++ b/Documentation/git-replay.adoc
>> @@ -88,6 +88,11 @@ incompatible with `--contained` (which is a modifier for `--onto` only).
>>  +
>>  The default mode can be configured via the `replay.refAction` configuration variable.
>>
>> +--linearize::
>> +       In this mode, `git replay` imitates `git rebase --no-rebase-merges`,
>> +       i.e. it cherry-picks only non-merge commits, each one on top of the
>> +       previous one.
>
> The SYNOPSIS block at the top of the file is missing this new flag.
>
> The replay_usage[] variable in cmd_replay is also missing this new flag.
>
>>  <revision-range>::
>>         Range of commits to replay; see "Specifying Ranges" in
>>         linkgit:git-rev-parse[1]. In `--advance=<branch>` or
>> diff --git a/builtin/replay.c b/builtin/replay.c
>> index 39e3a86f6c..fedfe46dc6 100644
>> --- a/builtin/replay.c
>> +++ b/builtin/replay.c
>> @@ -111,6 +111,8 @@ int cmd_replay(int argc,
>>                              N_("mode"),
>>                              N_("control ref update behavior (update|print)"),
>>                              PARSE_OPT_NONEG),
>> +               OPT_BOOL(0, "linearize", &opts.linearize,
>> +                        N_("ignore merge commits instead of replaying them")),
>
> "ignore" feels a bit ambiguous to me.  Can we use "drop" instead,
> matching your commit message?

Agreed, I don't like it too. "drop" sounds better.

>>                 OPT_END()
>>         };
>>
>> @@ -132,6 +134,8 @@ int cmd_replay(int argc,
>>                                   opts.contained, "--contained");
>>         die_for_incompatible_opt2(!!opts.ref, "--ref",
>>                                   !!opts.contained, "--contained");
>> +       die_for_incompatible_opt2(!!opts.revert, "--revert",
>> +                                 opts.linearize, "--linearize");
>
> Sensible; should the docs mention this incompatibility?  (I'm not sure
> myself; just throwing it out as food for thought.)

Let's add it.

>>
>>         /* Parse ref action mode from command line or config */
>>         ref_mode = get_ref_action_mode(repo, ref_action);
>> diff --git a/replay.c b/replay.c
>> index 7921d7dba3..81033fb889 100644
>> --- a/replay.c
>> +++ b/replay.c
>> @@ -277,12 +277,16 @@ static struct commit *pick_regular_commit(struct repository *repo,
>>                                           struct commit *onto,
>>                                           struct merge_options *merge_opt,
>>                                           struct merge_result *result,
>> +                                         struct commit *replayed_base,
>>                                           bool reverse,
>>                                           enum replay_empty_commit_action empty)
>>  {
>> -       struct commit *base, *replayed_base;
>> +       struct commit *base;
>>         struct tree *pickme_tree, *base_tree, *replayed_base_tree;
>>
>> +       if (replayed_base && reverse)
>> +               BUG("Linearizing commits is not supported when replaying in reverse");
>> +
>
> This is dead code given the die_for_incompatible_opt2 check above,
> right?  Just extra defense in depth?

We also have another defense-in-depth for --onto/--advance/--revert:

    BUG("expected one of onto_name, *advance_name, or *revert_name");

I don't mind having it for --linearize too.

>>         if (pickme->parents) {
>>                 base = pickme->parents->item;
>>                 base_tree = repo_get_commit_tree(repo, base);
>> @@ -291,7 +295,8 @@ static struct commit *pick_regular_commit(struct repository *repo,
>>                 base_tree = lookup_tree(repo, repo->hash_algo->empty_tree);
>>         }
>>
>> -       replayed_base = get_mapped_commit(replayed_commits, base, onto);
>> +       if (!replayed_base)
>> +               replayed_base = get_mapped_commit(replayed_commits, base, onto);
>>         replayed_base_tree = repo_get_commit_tree(repo, replayed_base);
>>         pickme_tree = repo_get_commit_tree(repo, pickme);
>>
>> @@ -430,12 +435,23 @@ int replay_revisions(struct rev_info *revs,
>>         while ((commit = get_revision(revs))) {
>>                 const struct name_decoration *decoration;
>>
>> -               if (commit->parents && commit->parents->next)
>> -                       die(_("replaying merge commits is not supported yet!"));
>> +               if (commit->parents && commit->parents->next) {
>> +                       if (!opts->linearize)
>> +                               die(_("replaying merge commits is not supported yet!"));
>> +                       /*
>> +                        * When linearizing, a merge commit itself is not picked,
>> +                        * but refs that point to it might need updating.
>> +                        */
>
> Is it worth pointing out that last_commit is intentionally not updated
> by this code path?  That is implied by your comment, but it takes a
> bit of reasoning to get there, and I think it might help future
> readers to just explicitly state it.

Ah yes, I didn't realize that, but you make a good point. I'll rephrase
the comment a bit.

>> +               } else {
>> +                       struct commit *to_pick = reverse ? last_commit : onto;
>> +                       last_commit =
>> +                               pick_regular_commit(revs->repo, commit,
>> +                                                   replayed_commits, to_pick,
>> +                                                   &merge_opt, &result,
>> +                                                   opts->linearize ? last_commit : NULL,
>> +                                                   reverse, opts->empty);
>> +               }
>>
>> -               last_commit = pick_regular_commit(revs->repo, commit, replayed_commits,
>> -                                                 reverse ? last_commit : onto,
>> -                                                 &merge_opt, &result, reverse, opts->empty);
>>                 if (!last_commit)
>>                         break;
>>
>> diff --git a/replay.h b/replay.h
>> index 1851a07705..07e6fdcca3 100644
>> --- a/replay.h
>> +++ b/replay.h
>> @@ -62,6 +62,11 @@ struct replay_revisions_options {
>>          * Defaults to REPLAY_EMPTY_COMMIT_DROP.
>>          */
>>         enum replay_empty_commit_action empty;
>> +
>> +       /*
>> +        * Whether to linearize the commits (i.e. drop merge commits).
>> +        */
>> +       int linearize;
>>  };
>>
>>  /* This struct is used as an out-parameter by `replay_revisions()`. */
>> diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
>> index 3353bc4a4d..64e0731188 100755
>> --- a/t/t3650-replay-basics.sh
>> +++ b/t/t3650-replay-basics.sh
>> @@ -565,4 +565,30 @@ test_expect_success '--onto with --ref rejects multiple revision ranges' '
>>         test_grep "cannot be used with multiple revision ranges" err
>>  '
>>
>> +test_expect_success 'replay merge commit fails' '
>> +       echo "fatal: replaying merge commits is not supported yet!" >expect &&
>> +       test_must_fail git replay --ref-action=print --onto main I..P 2>actual &&
>> +       test_cmp expect actual
>> +'
>> +
>> +test_expect_success 'replay to rebase merge commit with --linearize' '
>> +       git replay --ref-action=print --linearize --onto main I..topic-with-merge >result &&
>> +
>> +       test_line_count = 1 result &&
>> +
>> +       git log --format=%s $(cut -f 3 -d " " result) >actual &&
>> +       test_write_lines O N J M L B A >expect &&
>> +       test_cmp expect actual
>> +'
>> +
>> +test_expect_success 'replay to rebase merge commit with --linearize down to root commit' '
>> +       git replay --ref-action=print --linearize --onto main A..topic-with-merge >result &&
>
> You'd need to drop "A.." to have it go down to the root commit, as
> Junio mentioned elsewhere.

Yes, thanks for double confirmation.

>> +
>> +       test_line_count = 1 result &&
>> +
>> +       git log --format=%s $(cut -f 3 -d " " result) >actual &&
>> +       test_write_lines O N J I M L B A >expect &&
>> +       test_cmp expect actual
>> +'
>> +
>>  test_done
>
> Should there also be a testcase combining --linearize and --advance?

Sure.

> Should there be a test with the incompatibility of --revert &
> --linearize?  I think we have a few other tests for incompatible
> options.

I was already about to add that.

> One additional testing idea, borrowed from an older variant of
> this patch I had sitting in a local branch (dscho's original
> linearize patch, adapted): in addition to checking specific commit
> subjects, it's worth verifying that the linearized chain produces
> the *same patches* as the original.  Something along the lines of:
>
>         test_expect_success '--linearize preserves patches' '
>                 test_when_finished "git update-ref -d refs/heads/merge_I_L" &&
>                 test_tick &&
>                 git checkout -b merge_I_L I &&
>                 git merge --no-edit L &&
>
>                 git replay --linearize --onto A B..merge_I_L &&
>
>                 # range-diff ignores merges, so the original
>                 # {I, L, merge} reduces to {I, L} on the LHS,
>                 # and the replayed chain on the RHS should match.
>                 git range-diff B..merge_I_L@{1} B..merge_I_L >out &&
>                 ! test_grep -v "=" out &&
>
>                 git log --oneline A..merge_I_L >out &&
>                 test_line_count = 2 out
>         '
>
> The range-diff check is nice because it asserts patch equivalence
> rather than tying the test to a particular replay ordering, which
> makes the test less brittle if the rev-walk order ever changes.
> Feel free to take, adapt, or ignore.

Interesting idea and I like it. Lemme add it.

> Anyway, thanks for working on this; looking good.

Thanks!

-- 
Cheers,
Toon