Patchwork D527: rebase: move working parent and bookmark for obsoleted revs (BC)

login
register
mail settings
Submitter phabricator
Date Aug. 30, 2017, 1:40 a.m.
Message ID <82300545371c540c83039b895515311c@localhost.localdomain>
Download mbox | patch
Permalink /patch/23491/
State Not Applicable
Headers show

Comments

phabricator - Aug. 30, 2017, 1:40 a.m.
quark updated this revision to Diff 1421.
quark marked 3 inline comments as done.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D527?vs=1333&id=1421

REVISION DETAIL
  https://phab.mercurial-scm.org/D527

AFFECTED FILES
  hgext/rebase.py
  tests/test-rebase-obsolete.t

CHANGE DETAILS




To: quark, #hg-reviewers
Cc: martinvonz, mercurial-devel

Patch

diff --git a/tests/test-rebase-obsolete.t b/tests/test-rebase-obsolete.t
--- a/tests/test-rebase-obsolete.t
+++ b/tests/test-rebase-obsolete.t
@@ -205,8 +205,8 @@ 
   o  0:cd010b8cd998 A
   
   $ hg rebase --source 'desc(B)' --dest 'tip' --config experimental.rebaseskipobsolete=True
+  rebasing 8:8877864f1edb "B"
   note: not rebasing 9:08483444fef9 "D", already in destination as 11:4596109a6a43 "D"
-  rebasing 8:8877864f1edb "B"
   rebasing 10:5ae4c968c6ac "C"
   $ hg debugobsolete
   42ccdea3bb16d28e1848c95fe2e44c000f3f21b1 0 {cd010b8cd998f3981a5a8115f94f8da4ab506089} (*) {'user': 'test'} (glob)
@@ -736,8 +736,8 @@ 
   $ hg debugobsolete `hg log -r 7 -T '{node}\n'` --config experimental.stabilization=all
   obsoleted 1 changesets
   $ hg rebase -d 6 -r "4::"
+  rebasing 4:ff2c4d47b71d "C"
   note: not rebasing 7:360bbaa7d3ce "O", it has no successor
-  rebasing 4:ff2c4d47b71d "C"
   rebasing 8:8d47583e023f "P" (tip)
 
 If all the changeset to be rebased are obsolete and present in the destination, we
@@ -769,10 +769,8 @@ 
 If a rebase is going to create divergence, it should abort
 
   $ hg log -G
-  @  11:f44da1f4954c nonrelevant
+  @  10:121d9e3bc4c6 P
   |
-  | o  10:121d9e3bc4c6 P
-  |/
   o  9:4be60e099a77 C
   |
   o  6:9c48361117de D
@@ -904,7 +902,6 @@ 
   |
   ~
   $ hg rebase -r ".^^ + .^ + ." -d 19
-  note: not rebasing 21:8b31da3c4919 "dummy change", already in destination as 19:601db7a18f51 "dummy change successor"
   rebasing 20:b82fb57ea638 "willconflict second version"
   merging willconflict
   warning: conflicts while merging willconflict! (edit, then use 'hg resolve --mark')
@@ -916,6 +913,7 @@ 
   continue: hg rebase --continue
   $ hg rebase --continue
   rebasing 20:b82fb57ea638 "willconflict second version"
+  note: not rebasing 21:8b31da3c4919 "dummy change", already in destination as 19:601db7a18f51 "dummy change successor"
   rebasing 22:7bdc8a87673d "dummy change" (tip)
   $ cd ..
 
@@ -1062,8 +1060,8 @@ 
   > EOF
 
   $ hg rebase -d C -b F
+  rebasing 2:b18e25de2cf5 "D" (D)
   note: not rebasing 3:7fb047a69f22 "E" (E), already in destination as 1:112478962961 "B"
-  rebasing 2:b18e25de2cf5 "D" (D)
   rebasing 5:66f1a38021c9 "F" (F tip)
   note: rebase of 5:66f1a38021c9 created no changes to commit
   $ hg log -G
@@ -1179,8 +1177,8 @@ 
   > A B C  # D/D = D
   > EOS
   $ hg rebase -r B+A+D -d Z
+  rebasing 0:426bada5c675 "A" (A)
   note: not rebasing 1:fc2b737bb2e5 "B" (B), already in destination as 2:96cc3511f894 "C"
-  rebasing 0:426bada5c675 "A" (A)
   rebasing 3:b8ed089c80ad "D" (D)
 
   $ rm .hg/localtags
@@ -1226,17 +1224,46 @@ 
   2:1e9a3c00cbe9 b (no-eol)
   $ hg rebase -r 2 -d 3 --config experimental.stabilization.track-operation=1
   note: not rebasing 2:1e9a3c00cbe9 "b" (mybook), already in destination as 3:be1832deae9a "b"
-Check that working directory was not updated to rev 3 because rev 2 was skipped
-during the rebase operation
+Check that working directory and bookmark was updated to rev 3 although rev 2
+was skipped
   $ hg log -r .
-  2:1e9a3c00cbe9 b (no-eol)
-
-Check that bookmark was not moved to rev 3 if rev 2 was skipped during the
-rebase operation. This makes sense because if rev 2 has a successor, the
-operation generating that successor (ex. rebase) should be responsible for
-moving bookmarks. If the bookmark is on a precursor, like rev 2, that means the
-user manually moved it back. In that case we should not move it again.
+  3:be1832deae9a b (no-eol)
   $ hg bookmarks
-     mybook                    2:1e9a3c00cbe9
+     mybook                    3:be1832deae9a
   $ hg debugobsolete --rev tip
   1e9a3c00cbe90d236ac05ef61efcc5e40b7412bc be1832deae9ac531caa7438b8dcf6055a122cd8e 0 (*) {'user': 'test'} (glob)
+
+Obsoleted working parent and bookmark could be moved if an ancestor of working
+parent gets moved:
+
+  $ hg init $TESTTMP/ancestor-wd-move
+  $ cd $TESTTMP/ancestor-wd-move
+  $ hg debugdrawdag <<'EOS'
+  >  E D1  # rebase: D1 -> D2
+  >  | |
+  >  | C
+  > D2 |
+  >  | B
+  >  |/
+  >  A
+  > EOS
+  $ hg update D1 -q
+  $ hg bookmark book -i
+  $ hg rebase -r B+D1 -d E
+  rebasing 1:112478962961 "B" (B)
+  note: not rebasing 5:15ecf15e0114 "D1" (D1 tip book), already in destination as 2:0807738e0be9 "D2"
+  $ hg log -G -T '{desc} {bookmarks}'
+  @  B book
+  |
+  | x  D1
+  | |
+  o |  E
+  | |
+  | o  C
+  | |
+  o |  D2
+  | |
+  | x  B
+  |/
+  o  A
+  
diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -327,11 +327,7 @@ 
                   " unrebased descendants"),
                 hint=_('use --keep to keep original changesets'))
 
-        obsrevs = _filterobsoleterevs(self.repo, rebaseset)
-        self._handleskippingobsolete(obsrevs, destmap)
-
-        result = buildstate(self.repo, destmap, self.collapsef,
-                            self.obsoletenotrebased)
+        result = buildstate(self.repo, destmap, self.collapsef)
 
         if not result:
             # Empty state built, nothing to rebase
@@ -375,6 +371,10 @@ 
                         raise error.Abort(_('cannot collapse multiple named '
                             'branches'))
 
+        # Calculate self.obsoletenotrebased
+        obsrevs = _filterobsoleterevs(self.repo, self.state)
+        self._handleskippingobsolete(obsrevs, self.destmap)
+
         # Keep track of the active bookmarks in order to reset them later
         self.activebookmark = self.activebookmark or repo._activebookmark
         if self.activebookmark:
@@ -401,13 +401,34 @@ 
             desc = _ctxdesc(ctx)
             if self.state[rev] == rev:
                 ui.status(_('already rebased %s\n') % desc)
+            elif rev in self.obsoletenotrebased:
+                succ = self.obsoletenotrebased[rev]
+                if succ is None:
+                    msg = _('note: not rebasing %s, it has no '
+                            'successor\n') % desc
+                else:
+                    succctx = repo[succ]
+                    succdesc = '%d:%s "%s"' % (
+                        succctx.rev(), succctx,
+                        succctx.description().split('\n', 1)[0])
+                    msg = (_('note: not rebasing %s, already in '
+                             'destination as %s\n') % (desc, succdesc))
+                repo.ui.status(msg)
+                # Make clearrebased aware state[rev] is not a true successor
+                self.skipped.add(rev)
+                # Record rev as moved to its desired destination in self.state.
+                # This helps bookmark and working parent movement.
+                dest = max(adjustdest(repo, rev, self.destmap, self.state,
+                                      self.skipped))
+                self.state[rev] = dest
             elif self.state[rev] == revtodo:
                 pos += 1
                 ui.status(_('rebasing %s\n') % desc)
                 ui.progress(_("rebasing"), pos, ("%d:%s" % (rev, ctx)),
                             _('changesets'), total)
                 p1, p2, base = defineparents(repo, rev, self.destmap,
-                                             self.state)
+                                             self.state, self.skipped,
+                                             self.obsoletenotrebased)
                 self.storestatus(tr=tr)
                 storecollapsemsg(repo, self.collapsemsg)
                 if len(repo[None].parents()) == 2:
@@ -462,7 +483,8 @@ 
         repo, ui, opts = self.repo, self.ui, self.opts
         if self.collapsef and not self.keepopen:
             p1, p2, _base = defineparents(repo, min(self.state), self.destmap,
-                                          self.state)
+                                          self.state, self.skipped,
+                                          self.obsoletenotrebased)
             editopt = opts.get('edit')
             editform = 'rebase.collapse'
             if self.collapsemsg:
@@ -952,7 +974,7 @@ 
         copies.duplicatecopies(repo, rev, p1rev, skiprev=dest)
     return stats
 
-def adjustdest(repo, rev, destmap, state):
+def adjustdest(repo, rev, destmap, state, skipped):
     """adjust rebase destination given the current rebase state
 
     rev is what is being rebased. Return a list of two revs, which are the
@@ -1006,7 +1028,8 @@ 
     """
     # pick already rebased revs with same dest from state as interesting source
     dest = destmap[rev]
-    source = [s for s, d in state.items() if d > 0 and destmap[s] == dest]
+    source = [s for s, d in state.items()
+              if d > 0 and destmap[s] == dest and s not in skipped]
 
     result = []
     for prev in repo.changelog.parentrevs(rev):
@@ -1054,7 +1077,7 @@ 
         if s in nodemap:
             yield nodemap[s]
 
-def defineparents(repo, rev, destmap, state):
+def defineparents(repo, rev, destmap, state, skipped, obsskipped):
     """Return new parents and optionally a merge base for rev being rebased
 
     The destination specified by "dest" cannot always be used directly because
@@ -1081,7 +1104,7 @@ 
     dest = destmap[rev]
     oldps = repo.changelog.parentrevs(rev) # old parents
     newps = [nullrev, nullrev] # new parents
-    dests = adjustdest(repo, rev, destmap, state) # adjusted destinations
+    dests = adjustdest(repo, rev, destmap, state, skipped)
     bases = list(oldps) # merge base candidates, initially just old parents
 
     if all(r == nullrev for r in oldps[1:]):
@@ -1208,7 +1231,8 @@ 
             # If those revisions are covered by rebaseset, the result is good.
             # A merge in rebaseset would be considered to cover its ancestors.
             if siderevs:
-                rebaseset = [r for r, d in state.items() if d > 0]
+                rebaseset = [r for r, d in state.items()
+                             if d > 0 and r not in obsskipped]
                 merges = [r for r in rebaseset
                           if cl.parentrevs(r)[1] != nullrev]
                 unwanted[i] = list(repo.revs('%ld - (::%ld) - %ld',
@@ -1425,7 +1449,7 @@ 
         srcset -= set(result)
         yield result
 
-def buildstate(repo, destmap, collapse, obsoletenotrebased):
+def buildstate(repo, destmap, collapse):
     '''Define which revisions are going to be rebased and where
 
     repo: repo
@@ -1486,23 +1510,6 @@ 
         # if all parents of this revision are done, then so is this revision
         if parents and all((state.get(p) == p for p in parents)):
             state[rev] = rev
-    unfi = repo.unfiltered()
-    for r in obsoletenotrebased:
-        desc = _ctxdesc(unfi[r])
-        succ = obsoletenotrebased[r]
-        if succ is None:
-            msg = _('note: not rebasing %s, it has no successor\n') % desc
-            del state[r]
-            del destmap[r]
-        else:
-            destctx = unfi[succ]
-            destdesc = '%d:%s "%s"' % (destctx.rev(), destctx,
-                                       destctx.description().split('\n', 1)[0])
-            msg = (_('note: not rebasing %s, already in destination as %s\n')
-                   % (desc, destdesc))
-            del state[r]
-            del destmap[r]
-        repo.ui.status(msg)
     return originalwd, destmap, state
 
 def clearrebased(ui, repo, destmap, state, skipped, collapsedas=None):