Patchwork [14,of,17] rebase: refactor computerebase to case-by-case handling

login
register
mail settings
Submitter Mads Kiilerich
Date Nov. 30, 2014, 7:08 p.m.
Message ID <7e95d2f0e501ea79f776.1417374521@localhost.localdomain>
Download mbox | patch
Permalink /patch/6918/
State Changes Requested
Headers show

Comments

Mads Kiilerich - Nov. 30, 2014, 7:08 p.m.
# HG changeset patch
# User Mads Kiilerich <madski@unity3d.com>
# Date 1416364731 -3600
#      Wed Nov 19 03:38:51 2014 +0100
# Node ID 7e95d2f0e501ea79f77697c89247c2949ab987d8
# Parent  134387edbb48bbdac778267e5566fd02d2ebe421
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.

Patch

diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -28,6 +28,7 @@  import os, errno
 # nullrev = -1 means 'todo'
 nullmerge = -2 # revision is ignored when rebasing, use target
 revignored = -3 # revision is ignored when rebasing, use nearest ancestor
+outside = -4 # revision it outside (and thus not in state)
 
 cmdtable = {}
 command = cmdutil.command(cmdtable)
@@ -603,88 +604,96 @@  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\n') # untested
+            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\n') # untested
+            return target, rev, None, target, nullrev
+        if p2_ >= nullrev:
+            repo.ui.warn(' p1=nullrev, p2 rebased\n') # untested
+            return p2_, rev, p2, p2_, nullrev
+        repo.ui.warn(' p1=nullrev, p2 outside\n') # untested
+        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')
+            # TODO: This seems wrong. It should not use target.
+            # Parents should be p1_ and p2_.
+            # The merge should also be 4-way and also use p2 :-(
+            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
@@ -231,6 +231,7 @@  Check that the right ancestors is used w
    skipping null merge of 8:8e4e2c1a07ae
   rebasing 9:e31216eec445 "more changes to f1"
   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
@@ -256,6 +257,7 @@  Check that the right ancestors is used w
    rebased 9:e31216eec445 as 19c888675e13
   rebasing 10:2f2496ddf49d "merge" (tip)
   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