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

[Junio C Hamano <gitster@pobox.com>]

Toon Claes <toon@iotcl.com> writes:

>  Documentation/git-replay.adoc |  8 ++++-
>  builtin/replay.c              |  6 +++-
>  replay.c                      | 50 ++++++++++++++++----------
>  replay.h                      |  5 +++
>  t/t3650-replay-basics.sh      | 84 ++++++++++++++++++++++++++++++++++++++++++-
>  5 files changed, 132 insertions(+), 21 deletions(-)

"replay --linearize" behaves differently from the flattening rebase
in a case where X and Y that forked from A are merged at Z, and we
ask to flatten the history leading to Z, doesn't it?

     A----X
      \    \
       Y----Z (tip)

A typical flattening rebase would rewrite X to X', Y to Y', while
dropping Z, and would leave us a flattened history, like

     A---X'---Y' (updated tip, the order of X' and Y' may be swapped)

I may be misreading the logic, but doesn't "replay --linearize"
instead produce

     A----X' (dangling)
      \ 
       Y' (tip -- Z is dropped and gets mapped)

and leave X' dangling (or Y'; the point is that only one of them
will survive), never incorporating it in the resulting history?

> +		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, leave
> +			 * `last_commit` unchanged, and fall through to the
> +			 * rest of the loop. As a result:
> +			 * - the merge commit is mapped to `last_commit` in
> +			 *   `replayed_commits`, this will become the parent for
> +			 *   the child commits.
> +			 * - refs previously pointing to the merge commit are
> +			 *   rewritten to point to the previous non-merge commit.
> +			 */
> +		} else {
> +			/*
> +			 * pick_regular_commit() looks up the parent of `commit` in
> +			 * `replayed_commits` to determine the ancestor to replay onto.
> +			 * The `default_base` parameter is used when no ancestor is found,
> +			 * which happens for the first commit in the revision range.
> +			 * When reverting, commits are replayed in reverse order, so the
> +			 * lookup never succeeds, and we need to pass `last_commit`.
> +			 */
> +			struct commit *base = onto;
> +			if (mode == REPLAY_MODE_REVERT)
> +				base = last_commit;
> +
> +			last_commit = pick_regular_commit(revs->repo, commit, base,
> +							  replayed_commits,
> +							  &merge_opt, &result,
> +							  mode, opts->empty);
> +		}
> +
>  		if (!last_commit)
>  			break;

Immediately after this hunk beyond the post-context are these lines.

		/* Record commit -> last_commit mapping */
		put_mapped_commit(replayed_commits, commit, last_commit);

Let's imagine X gets processed first. X (and other commits on its
branch) gets replayed, last_commit is set to X' (which is the
rewritten X).  replayed_commits mapping holds X->X' mapping.

Then let's imagine the history leading to Y is replayed next.
last_commit becomes Y', and Y->Y' mapping is stored in
replayed_commits.

Finally, we see Z.  We are going to _drop_ it.  last_commit is left
unchanged, pointing at Y'.  Then last_commit (i.e., Y') is used as
the merge commit Z maps to (i.e., correctly dropping Z).

Any descendants of Z, if any, will be grafted as descendants of Y'.
If X did not have any descendants other than Z in the rewritten part
of the history, then X' (and commits leading to it) would be lost,
no?

This "loss of the other branch" may be an inherent characteristic of
this feature (i.e., I do not think it is necessarily a bug, and it
may even be that the "bug" is in the way I am reading the patch),
but then I wonder if the user may want to have control over which
side branch should survive, perhaps?  It would probably need to be
documented, and a test or two to cast this behaviour in stone.

> diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
> index 3353bc4a4d..34c038eab9 100755
> --- a/t/t3650-replay-basics.sh
> +++ b/t/t3650-replay-basics.sh
> @@ -52,8 +52,12 @@ test_expect_success 'setup' '

The pre-context here has

	git switch --detach topic4 &&
	test_commit N &&
	test_commit O &&
	git switch -c topic-with-merge topic4 &&

>  	test_merge P O --no-ff &&
>  	git switch main &&

The above does prepare topic-with-merge branch, but ...

> +test_expect_success 'replay to rebase merge commit with --linearize' '
> +	git replay --ref-action=print --linearize \
> +		--onto main I..topic-with-merge >result &&

... this does not really exersize linearizing replay in a typical
mergy history.  P merges O with --no-ff because otherwise there
won't be a merge, since O is a descendant of the commit "test_merge
P O" runs on (i.e., topic4 == topic-with-merge).

    topic4 --- N --- O
          \           \
           .-----------P

So, as long as O is replayed later than the parent of N (which is
true), O' will be the surviving tip (corresponds to Y' that the
dropped Z was mapped to in the earlier example), and nothing gets
orphaned, I think.

Perhaps a test to try a real merge may look something like this.

diff --git c/t/t3650-replay-basics.sh w/t/t3650-replay-basics.sh
index 34c038eab9..bb737f729a 100755
--- c/t/t3650-replay-basics.sh
+++ w/t/t3650-replay-basics.sh
@@ -647,4 +647,37 @@ test_expect_success 'replay with --linearize to rebase multiple divergent branch
 	test_cmp expect actual
 '
 
+test_expect_success 'replay with --linearize of a divergent merge drops one branch' '
+	git switch -c topic-divergent-base main &&
+	test_commit base &&
+	# Fork 1: base -> X
+	git switch -c topic-divergent-x &&
+	test_commit X &&
+	# Fork 2: base -> Y
+	git switch topic-divergent-base &&
+	git switch -c topic-divergent-y &&
+	test_commit Y &&
+	# Merge them at Z
+	git switch topic-divergent-x &&
+	test_merge Z topic-divergent-y --no-ff &&
+
+	# History is now:
+	#
+	#       X - Z (topic-divergent-x)
+	#      /   /
+	#  base - Y
+	#
+
+	git replay --ref-action=print --linearize \
+		--onto main topic-divergent-base..topic-divergent-x >result &&
+	test_line_count = 1 result &&
+	tip=$(cut -f 3 -d " " result) &&
+	# Get the commits replayed onto main
+	git log --format=%s main..$tip >actual &&
+	# We expect exactly one commit to be replayed (either X or Y)
+	# because the other one is left dangling due to the merge being dropped.
+	test_line_count = 1 actual &&
+	test_grep "^[XY]$" actual
+'
+
 test_done