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

Patrick Steinhardt [off-list ref] writes:

git-rebase(1) essentially knows about three different modes:

  - "--no-rebase-merges", which is the default and maps to your
    "--linearize".

  - "--rebase-merges", which by default doesn't rebase cousins by using
    "--ancestry-path" internally.

  - "--rebase-merges=rebase-cousins", which doesn't pass the above
    option.

So it's not a simple boolean there, which makes me wonder whether we
should mirror the same interface so that all of git-rebase(1)'s modes
can be represented, as well.

That's a valid question, although I don't know a good answer to that.

Basically you're asking for what the command line options will look
like? Allow me to think out loud.

In this series I'm adding --linearize to git-replay(1). As mentioned, I
don't think it makes sense to add it to git-history(1) as well. Without
this option, the process aborts when it encounters a merge.

Dscho sent a patch series to properly replay (2-way) merges. I think
this should become the default for both git-replay(1) and
git-history(1).

But then, do we want to have an option that brings back the current
behavior of aborting at merges? Maybe with --no-merges?

Then there's the option of rebasing cousins left. That's something that
isn't covered by Dscho's series yet. Maybe --replay-cousins?

To reiterate what the final design could look like:

 * <nothing>: replay merges preserving topology.
 * "--linearize": flattens merges (only git-replay(1)).
 * "--no-merges": dies when the process tries to replay a merge.
 * "--replay-cousins": does what --rebase-merges=rebase-cousins does.

Now, all these options are (I think) mutually exclusive, so we could
consider an option "--replay-merges=<mode>", but personally I find
"--<option>=<value>" arguments harder to use than specifying separate
options.

I think I'm avoiding your question, because the design of the command
line parameters doesn't need tot 1-on-1 correlate to the internal
datastructure. And I agree the mode isn't a boolean, but does that mean
we want to use an enum internally? Well, I don't know. And I also don't
think that matters right now. Code is easy to change, I think the
command line options should be designed with the future in mind, which I
believe we do with "--linearize".

Sorry for this long-winded rambling, but bottom line I think it's fine
to add --linearize and in the future add more options and see how the
code should evolve to support those.

quoted

diff --git a/replay.c b/replay.c
index 7921d7dba3..5539daff00 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");

Nit: Error messages should typically start with a lower-case letter.

Thanks.

quoted

@@ -430,12 +435,25 @@ 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!"));
+			/*
+			 * Drop the merge commit: do not pick it and leave
+			 * last_commit unchanged, so its children (and any ref
+			 * pointing at it) are reparented onto the previous
+			 * non-merge commit, which the ref-update loop below uses.
+			 */

One could add a hint here that tells the user to pass the option. But I
guess that might be somewhat weird, as we cannot assume that we're
called by git-replay(1) here.

Yeah, true...

-- 
Cheers,
Toon