[PATCH 4 of 5 v3] rebase: refactor computerebase to case-by-case handling

Mads Kiilerich mads at kiilerich.com
Wed Dec 3 21:12:48 CST 2014


# HG changeset patch
# User Mads Kiilerich <madski at unity3d.com>
# Date 1416364731 -3600
#      Wed Nov 19 03:38:51 2014 +0100
# Node ID 742988c7a05cb1bd49f9be0f057a9c8ef4e20d95
# Parent  e031c5201577b359af981396957a0b9a9a5f5300
rebase: refactor computerebase to case-by-case handling

This makes it simpler to review and tweak handling of different cases.

This implementation was created using the old implementation as black box. It
started out with how I think it should be, and was tweaked until it matched how
it actually is. Further tweaking can take us close to where we want to be, step
by step.

The only case where the test suite coverage gives different result from
computerebase is in test-rebase-obsolete.t in a case where None as ancestor
would give the same ancestor as the one previously explicitly specified.

diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -25,6 +25,7 @@ import os, errno
 
 nullmerge = -2
 revignored = -3
+outside = -4 # in computerebase: not in state, thus outside rebase set
 
 cmdtable = {}
 command = cmdutil.command(cmdtable)
@@ -586,88 +587,93 @@ def computerebase(repo, rev, target, sta
     '''Return the merge revisions and new parent relationship of rev rebased
     to target:
         merge parent 1, merge parent 2, ancestor, new parent 1, new parent 2
+
+    The following table shows how it is - not necessarily how it should be.
+
+    Rebasing of rev with parents p1 and p2 onto target as rev' is written:
+        m1,m2|a
+        p1',p2'
+
+        \  p2     null         ancestor        rebased        outside
+      p1 \
+    null       target,rev|  target,rev|None  p2',rev|p2    target,rev|p2
+                 target         target           p2'          target
+
+    ancestor target,rev|Non target,rev|None  p2',rev|p2   target,rev|None
+                 target         target           p2'         target,p2
+
+    rebased    p1',rev|p1     p1',rev|p1    target,rev|p1   p1',rev|p1
+                   p1'            p1'          p1',p2         p1',p2
+
+    outside   target,rev|p1 target,rev|None  p2',rev|p2       abort
+                 target        target,p1       p2',p1       3 parents
     '''
-    parents = repo[rev].parents()
-    p1 = p2 = nullrev
+    ctx = repo[rev]
 
-    p1n = parents[0].rev()
-    if p1n in targetancestors:
-        p1 = target
-    elif p1n in state:
-        if state[p1n] == nullmerge:
-            p1 = target
-        elif state[p1n] == revignored:
-            p1 = nearestrebased(repo, p1n, state)
-            if p1 is None:
-                p1 = target
-        else:
-            p1 = state[p1n]
-    else: # p1n external
-        p1 = target
-        p2 = p1n
+    p1 = ctx.p1().rev()
+    p1_ = state.get(p1, outside)
+    if p1_ == revignored:
+        p1_ = nearestrebased(repo, p1, state)
+        if p1_ is None:
+            p1_ = target
 
-    if len(parents) == 2 and parents[1].rev() not in targetancestors:
-        p2n = parents[1].rev()
-        # interesting second parent
-        if p2n in state:
-            if p1 == target: # p1n in targetancestors or external
-                p1 = state[p2n]
-            elif state[p2n] == revignored:
-                p2 = nearestrebased(repo, p2n, state)
-                if p2 is None:
-                    # no ancestors rebased yet, detach
-                    p2 = target
-            else:
-                p2 = state[p2n]
-        else: # p2n external
-            if p2 != nullrev: # p1n external too => rev is a merged revision
-                raise util.Abort(_('cannot use revision %d as base, result '
-                        'would have 3 parents') % rev)
-            p2 = p2n
+    p2 = ctx.p2().rev()
+    p2_ = state.get(p2, outside)
+    if p2_ == revignored:
+        p2_ = nearestrebased(repo, p2, state)
+        if p2_ is None:
+            p2_ = target
 
-    if rev == min(state):
-        # Case (1) initial changeset of a non-detaching rebase.
-        # Let the merge mechanism find the base itself.
-        base = None
-    elif not repo[rev].p2():
-        # 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.
-        #
-        # Imagine we have:
-        # - M: currently rebase revision in this step
-        # - A: one parent of M
-        # - B: second 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
-        #
-        # If we pick B as the base, the merge involves:
-        # - changes from B to M (actual changeset payload)
-        # - changes from B to D (induced by rebase) as D is a rebased
-        #   version of B)
-        # Which exactly represent the rebase operation.
-        #
-        # If we pick the 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.
-        for p in repo[rev].parents():
-            if state.get(p.rev()) == p1:
-                base = p.rev()
-                break
-        else: # fallback when base not found
-            base = None
-
-            # Raise because this function is called wrong (see issue 4106)
-            raise AssertionError('no base found to rebase on '
-                                 '(computerebase called wrong)')
-    return p1, rev, base, p1, p2
+    if p1 == nullrev:
+        if p2 == nullrev:
+            # Currently not possible because 'nothing to rebase from ...'
+            repo.ui.warn(' p1=nullrev, p2=nullrev - untested\n')
+            return target, rev, nullrev, target, nullrev
+        # These can't happen with normal hg but might happen anyway:
+        if p2 in targetancestors:
+            repo.ui.warn(' p1=nullrev, p2 ancestor - untested\n')
+            return target, rev, None, target, nullrev
+        if p2_ >= nullrev:
+            repo.ui.warn(' p1=nullrev, p2 rebased - untested\n')
+            return p2_, rev, p2, p2_, nullrev
+        repo.ui.warn(' p1=nullrev, p2 outside - untested\n')
+        return target, rev, p2, target, nullrev
+    if p1 in targetancestors:
+        if p2 == nullrev:
+            repo.ui.debug(' p1 ancestor, p2=nullrev\n')
+            return target, rev, None, target, nullrev
+        if p2 in targetancestors:
+            repo.ui.debug(' p1 ancestor, p2 ancestor\n')
+            return target, rev, None, target, nullrev
+        if p2_ >= nullrev:
+            repo.ui.debug(' p1 ancestor, p2 rebased\n')
+            return p2_, rev, p2, p2_, nullrev
+        repo.ui.debug(' p1 ancestor, p2 outside\n')
+        return target, rev, None, target, p2
+    if p1_ >= nullrev:
+        if p2 == nullrev:
+            repo.ui.debug(' p1 rebased, p2=nullrev\n')
+            return p1_, rev, p1, p1_, nullrev
+        if p2 in targetancestors:
+            repo.ui.debug(' p1 rebased, p2 ancestor\n')
+            return p1_, rev, p1, p1_, nullrev
+        if p2_ >= nullrev:
+            repo.ui.debug(' p1 rebased, p2 rebased\n')
+            return target, rev, p1, target, nullrev
+        repo.ui.debug(' p1 rebased, p2 outside\n')
+        return p1_, rev, p1, p1_, p2
+    if p2 == nullrev:
+        repo.ui.debug(' p1 outside, p2=nullrev\n')
+        return target, rev, p1, target, nullrev
+    if p2 in targetancestors:
+        repo.ui.debug(' p1 outside, p2 ancestor\n')
+        return target, rev, None, target, p1
+    if p2_ >= nullrev:
+        repo.ui.debug(' p1 outside, p2 rebased\n')
+        return p2_, rev, p2, p2_, p1
+    repo.ui.debug(' p1 outside, p2 outside\n')
+    raise util.Abort(_('cannot use revision %d as base, result '
+                       'would have 3 parents') % rev)
 
 def isagitpatch(repo, patchname):
     'Return true if the given patch is in git format'
diff --git a/tests/test-rebase-conflicts.t b/tests/test-rebase-conflicts.t
--- a/tests/test-rebase-conflicts.t
+++ b/tests/test-rebase-conflicts.t
@@ -213,6 +213,7 @@ Check that the right ancestors is used w
   $ hg rebase -s9 -d2 --debug # use debug to really check merge base used
   rebase onto 2 starting from e31216eec445
   rebasing: 9:e31216eec445 5/6 changesets (83.33%)
+   p1 outside, p2=nullrev
    merging 2 and 9 using ancestor 8, setting parents 2 and -1
   rebase status stored
    update to 2:4bc80088dc6b
@@ -236,6 +237,7 @@ Check that the right ancestors is used w
   updating: f1.txt 1/1 files (100.00%)
   f1.txt
   rebasing: 10:2f2496ddf49d 6/6 changesets (100.00%)
+   p1 outside, p2 rebased
    merging 11 and 10 using ancestor 9, setting parents 11 and 7
   rebase status stored
    already in target


More information about the Mercurial-devel mailing list