[PATCH] rebase: clarify comment about merge ancestor when rebasing merges

[Mads Kiilerich]

# HG changeset patch
# User Mads Kiilerich <madski at unity3d.com>
# Date 1420331347 -3600
#      Sun Jan 04 01:29:07 2015 +0100
# Node ID c9d31cdeb6cc91c745d570b542aff07c2cac47e6
# Parent  ff6694de19f3c7f3829ef65f504fb3ad573deb70
rebase: clarify comment about merge ancestor when rebasing merges

The code for picking a merge ancestor when rebasing merges had a long and
incorrect comment.

The comment would perhaps have been fine as commit message but do not make the
code more readable or maintainable and is a bad substitute for correct and
readable code.

The correct essense of the comment is quite trivial: a merge of an ancestor of
the rebase destination and an 'outside' revision can be rebased as if it was a
linear change, using 'destination ancestor parent' as base and pretty much
ignoring the 'outside' revision.

The code path where the comment is placed is however also used for other kinds
of merge rebases. The comment is thus not really correct and not helpful. I
think it would be better to drop the comment and rewrite the code.

diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -645,17 +645,19 @@ def defineparents(repo, rev, target, sta
         # Case (2) detaching the node with a single parent, use this parent
         base = repo[rev].p1().rev()
     else:
-        # In case of merge, we need to pick the right parent as merge base.
+        # Assuming there is a p1, this is the case where there also is a p2.
+        # We are thus rebasing a merge and need to pick the right merge base.
         #
         # Imagine we have:
-        # - M: currently rebase revision in this step
+        # - M: current rebase revision in this step
         # - A: one parent of M
-        # - B: second parent of M
+        # - B: other parent of M
         # - D: destination of this merge step (p1 var)
         #
-        # If we are rebasing on D, D is the successors of A or B. The right
-        # merge base is the one D succeed to. We pretend it is B for the rest
-        # of this comment
+        # Consider the case where D is a descendant of A or B and the other is
+        # 'outside'. In this case, the right merge base is the D ancestor.
+        #
+        # An informal proof, assuming A is 'outside' and B is the D ancestor:
         #
         # If we pick B as the base, the merge involves:
         # - changes from B to M (actual changeset payload)
@@ -663,12 +665,20 @@ def defineparents(repo, rev, target, sta
         #   version of B)
         # Which exactly represent the rebase operation.
         #
-        # If we pick the A as the base, the merge involves
+        # If we pick A as the base, the merge involves:
         # - changes from A to M (actual changeset payload)
         # - changes from A to D (with include changes between unrelated A and B
         #   plus changes induced by rebase)
         # Which does not represent anything sensible and creates a lot of
-        # conflicts.
+        # conflicts. A is thus not the right choice - B is.
+        #
+        # Note: The base found in this 'proof' is only correct in the specified
+        # case. This base do not make sense if D not is a descendant of A or B
+        # or if the other parent not is 'outside' (especially not if the other
+        # parent has been rebased). The current implementation do however not
+        # make it feasible to consider different cases separately. In these
+        # other changes we currently just leave it to the user to correctly
+        # resolve an impossible merge using a wrong ancestor.
         for p in repo[rev].parents():
             if state.get(p.rev()) == p1:
                 base = p.rev()