From: Toon Claes Date: Tue, 16 Jun 2026 07:09:16 GMT Subject: Re: [PATCH v2 3/3] replay: offer an option to linearize the commit topology Message-ID: <874ij3ynab.fsf@emacs.iotcl.com> In-Reply-To: Elijah Newren writes: > Hi, > > On Wed, Jun 10, 2026 at 7:51 AM Toon Claes wrote: >> >> From: Johannes Schindelin >> >> 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 >> Signed-off-by: Johannes Schindelin >> Signed-off-by: Toon Claes >> --- >> 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. > >> :: >> Range of commits to replay; see "Specifying Ranges" in >> linkgit:git-rev-parse[1]. In `--advance=` 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