Patchwork D727: rebase: move bookmarks with --keep (issue5682)

login
register
mail settings
Submitter phabricator
Date Sept. 18, 2017, 7:03 p.m.
Message ID <88f9ac1bc8b505bf2ff35dde8bfca2cb@localhost.localdomain>
Download mbox | patch
Permalink /patch/23988/
State Not Applicable
Headers show

Comments

phabricator - Sept. 18, 2017, 7:03 p.m.
quark updated this revision to Diff 1868.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D727?vs=1867&id=1868

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

AFFECTED FILES
  hgext/rebase.py
  mercurial/scmutil.py
  tests/test-rebase-bookmarks.t
  tests/test-rebase-emptycommit.t

CHANGE DETAILS




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

Patch

diff --git a/tests/test-rebase-emptycommit.t b/tests/test-rebase-emptycommit.t
--- a/tests/test-rebase-emptycommit.t
+++ b/tests/test-rebase-emptycommit.t
@@ -47,30 +47,35 @@ 
   |/
   o  0 A
   
-With --keep, bookmark should not move
+With --keep, bookmark should move
 
   $ hg rebase -r 3+4 -d E --keep
   rebasing 3:e7b3f00ed42e "D" (BOOK-D)
   note: rebase of 3:e7b3f00ed42e created no changes to commit
   rebasing 4:69a34c08022a "E" (BOOK-E)
   note: rebase of 4:69a34c08022a created no changes to commit
   $ hg log -G -T '{rev} {desc} {bookmarks}'
-  o  7 E
+  o  7 E BOOK-D BOOK-E
   |
   o  6 D
   |
   | o  5 F BOOK-F
   | |
-  | o  4 E BOOK-E
+  | o  4 E
   | |
-  | o  3 D BOOK-D
+  | o  3 D
   | |
   | o  2 C BOOK-C
   | |
   o |  1 B
   |/
   o  0 A
   
+Move D and E back for the next test
+
+  $ hg bookmark BOOK-D -fqir 3
+  $ hg bookmark BOOK-E -fqir 4
+
 Bookmark is usually an indication of a head. For changes that are introduced by
 an ancestor of bookmark B, after moving B to B-NEW, the changes are ideally
 still introduced by an ancestor of changeset on B-NEW. In the below case,
diff --git a/tests/test-rebase-bookmarks.t b/tests/test-rebase-bookmarks.t
--- a/tests/test-rebase-bookmarks.t
+++ b/tests/test-rebase-bookmarks.t
@@ -1,6 +1,7 @@ 
   $ cat >> $HGRCPATH <<EOF
   > [extensions]
   > rebase=
+  > drawdag=$TESTDIR/drawdag.py
   > 
   > [phases]
   > publish=False
@@ -210,3 +211,35 @@ 
   rebasing 6:f677a2907404 "bisect2"
   rebasing 7:325c16001345 "bisect3" (tip bisect)
   saved backup bundle to $TESTTMP/a3/.hg/strip-backup/345c90f326a4-b4840586-rebase.hg (glob)
+
+Bookmark and working parent get moved even if --keep is set (issue5682)
+
+  $ hg init $TESTTMP/book-keep
+  $ cd $TESTTMP/book-keep
+  $ hg debugdrawdag <<'EOS'
+  > B C
+  > |/
+  > A
+  > EOS
+  $ eval `hg tags -T 'hg bookmark -ir {node} {tag};\n' | grep -v tip`
+  $ rm .hg/localtags
+  $ hg up -q B
+  $ hg tglog
+  o  2: 'C' bookmarks: C
+  |
+  | @  1: 'B' bookmarks: B
+  |/
+  o  0: 'A' bookmarks: A
+  
+  $ hg rebase -r B -d C --keep
+  rebasing 1:112478962961 "B" (B)
+  $ hg tglog
+  @  3: 'B' bookmarks: B
+  |
+  o  2: 'C' bookmarks: C
+  |
+  | o  1: 'B' bookmarks:
+  |/
+  o  0: 'A' bookmarks: A
+  
+
diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
--- a/mercurial/scmutil.py
+++ b/mercurial/scmutil.py
@@ -585,35 +585,55 @@ 
     That includes writing obsmarkers or stripping nodes, and moving bookmarks.
     (we might also want to move working directory parent in the future)
 
-    mapping is {oldnode: [newnode]} or a iterable of nodes if they do not have
-    replacements. operation is a string, like "rebase".
+    mapping is {oldnode: {'new': [newnode], 'move': newnode}}, where 'new'
+    decide obsolete markers, and 'move' decide bookmark move, which could be
+    different from new. If 'move' is not provided, it will be calculated
+    automatically from 'new'. If 'new' is not set, the oldnode will not be
+    removed or obsoleted.
+
+    To make the API easier to use, mapping could also be in the following
+    forms:
+
+        - {oldnode: [newnode]}, equal to {oldnode: {'new': [newnode]}}
+        - {oldnode}, equal to {oldnode: []}
     """
+    if not mapping:
+        return
+
+    # translate {oldnode} form
     if not util.safehasattr(mapping, 'items'):
-        mapping = {n: () for n in mapping}
+        mapping = {n: {'new': ()} for n in mapping}
+    elif all('new' not in v and 'move' not in v for v in mapping.values()):
+        mapping = {n: {'new': newnodes} for n, newnodes in mapping.items()}
 
     with repo.transaction('cleanup') as tr:
         # Move bookmarks
         bmarks = repo._bookmarks
         bmarkchanges = []
-        allnewnodes = [n for ns in mapping.values() for n in ns]
-        for oldnode, newnodes in mapping.items():
+        allnewnodes = [n for v in mapping.values() for n in v.get('new', ())]
+        for oldnode, value in mapping.items():
             oldbmarks = repo.nodebookmarks(oldnode)
             if not oldbmarks:
                 continue
+            newnode = value.get('move')
             from . import bookmarks # avoid import cycle
-            if len(newnodes) > 1:
-                # usually a split, take the one with biggest rev number
-                newnode = next(repo.set('max(%ln)', newnodes)).node()
-            elif len(newnodes) == 0:
-                # move bookmark backwards
-                roots = list(repo.set('max((::%n) - %ln)', oldnode,
-                                      list(mapping)))
-                if roots:
-                    newnode = roots[0].node()
+            if not newnode:
+                newnodes = value.get('new')
+                if newnodes is None:
+                    continue
+                if len(newnodes) > 1:
+                    # usually a split, take the one with biggest rev number
+                    newnode = next(repo.set('max(%ln)', newnodes)).node()
+                elif len(newnodes) == 0:
+                    # move bookmark backwards
+                    roots = list(repo.set('max((::%n) - %ln)', oldnode,
+                                          list(mapping)))
+                    if roots:
+                        newnode = roots[0].node()
+                    else:
+                        newnode = nullid
                 else:
-                    newnode = nullid
-            else:
-                newnode = newnodes[0]
+                    newnode = newnodes[0]
             repo.ui.debug('moving bookmarks %r from %s to %s\n' %
                           (oldbmarks, hex(oldnode), hex(newnode)))
             # Delete divergent bookmarks being parents of related newnodes
@@ -641,13 +661,16 @@ 
             isobs = unfi.obsstore.successors.__contains__
             torev = unfi.changelog.rev
             sortfunc = lambda ns: torev(ns[0])
-            rels = [(unfi[n], tuple(unfi[m] for m in s))
-                    for n, s in sorted(mapping.items(), key=sortfunc)
-                    if s or not isobs(n)]
-            obsolete.createmarkers(repo, rels, operation=operation)
+            rels = [(unfi[n], tuple(unfi[m] for m in v['new']))
+                    for n, v in sorted(mapping.items(), key=sortfunc)
+                    if 'new' in v and (v['new'] or not isobs(n))]
+            if rels:
+                obsolete.createmarkers(repo, rels, operation=operation)
         else:
             from . import repair # avoid import cycle
-            repair.delayedstrip(repo.ui, repo, list(mapping), operation)
+            tostrip = [n for n, v in mapping.items() if 'new' in v]
+            if tostrip:
+                repair.delayedstrip(repo.ui, repo, tostrip, operation)
 
 def addremove(repo, matcher, prefix, opts=None, dry_run=None, similarity=None):
     if opts is None:
diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -543,12 +543,12 @@ 
             ui.note(_("update back to initial working directory parent\n"))
             hg.updaterepo(repo, newwd, False)
 
+        collapsedas = None
         if not self.keepf:
-            collapsedas = None
             if self.collapsef:
                 collapsedas = newnode
-            clearrebased(ui, repo, self.destmap, self.state, self.skipped,
-                         collapsedas)
+        clearrebased(ui, repo, self.destmap, self.state, self.skipped,
+                     collapsedas, self.keepf)
 
         clearstatus(repo)
         clearcollapsemsg(repo)
@@ -1512,31 +1512,29 @@ 
             state[rev] = rev
     return originalwd, destmap, state
 
-def clearrebased(ui, repo, destmap, state, skipped, collapsedas=None):
+def clearrebased(ui, repo, destmap, state, skipped, collapsedas=None,
+                 keepf=False):
     """dispose of rebased revision at the end of the rebase
 
     If `collapsedas` is not None, the rebase was a collapse whose result if the
-    `collapsedas` node."""
+    `collapsedas` node.
+
+    If `keepf` is not True, the rebase has --keep set and no nodes should be
+    removed (but bookmarks still need to be moved).
+    """
     tonode = repo.changelog.node
-    # Move bookmark of skipped nodes to destination. This cannot be handled
-    # by scmutil.cleanupnodes since it will treat rev as removed (no successor)
-    # and move bookmark backwards.
-    bmchanges = [(name, tonode(state[rev]))
-                 for rev in skipped
-                 for name in repo.nodebookmarks(tonode(rev))]
-    if bmchanges:
-        with repo.transaction('rebase') as tr:
-            repo._bookmarks.applychanges(repo, tr, bmchanges)
     mapping = {}
     for rev, newrev in sorted(state.items()):
         if newrev >= 0 and newrev != rev:
-            if rev in skipped:
-                succs = ()
-            elif collapsedas is not None:
-                succs = (collapsedas,)
-            else:
-                succs = (tonode(newrev),)
-            mapping[tonode(rev)] = succs
+            newnode = collapsedas or tonode(newrev)
+            value = {'move': newnode}
+            if not keepf:
+                if rev in skipped:
+                    succs = ()
+                else:
+                    succs = (newnode,)
+                value['new'] = succs
+            mapping[tonode(rev)] = value
     scmutil.cleanupnodes(repo, mapping, 'rebase')
 
 def pullrebase(orig, ui, repo, *args, **opts):