D5391: cleanupnodes: trust caller when "moves" is not None

[martinvonz (Martin von Zweigbergk)]

martinvonz created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  If "moves" (indicating how to move bookmarks) is None, we fill it out
  based on "replacements" (indicating which obsmarkers to add). If
  "moves" is not None, we would still add items based on
  "replacements". This makes it impossible to pass "moves={}" and not
  move bookmarks, which surprised me. The only caller that currently
  passes a value for "moves" was the rebase extension and there we were
  already adding bookmark moves corresponding to obsmarker additions, so
  it should not be impacted.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/scmutil.py

CHANGE DETAILS

diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
--- a/mercurial/scmutil.py
+++ b/mercurial/scmutil.py
@@ -901,32 +901,33 @@
             repls[key] = value
         replacements = repls
 
+    # Unfiltered repo is needed since nodes in replacements might be hidden.
+    unfi = repo.unfiltered()
+
     # Calculate bookmark movements
     if moves is None:
         moves = {}
-    # Unfiltered repo is needed since nodes in replacements might be hidden.
-    unfi = repo.unfiltered()
-    for oldnodes, newnodes in replacements.items():
-        for oldnode in oldnodes:
-            if oldnode in moves:
-                continue
-            if len(newnodes) > 1:
-                # usually a split, take the one with biggest rev number
-                newnode = next(unfi.set('max(%ln)', newnodes)).node()
-            elif len(newnodes) == 0:
-                # move bookmark backwards
-                allreplaced = []
-                for rep in replacements:
-                    allreplaced.extend(rep)
-                roots = list(unfi.set('max((::%n) - %ln)', oldnode,
-                                      allreplaced))
-                if roots:
-                    newnode = roots[0].node()
+        for oldnodes, newnodes in replacements.items():
+            for oldnode in oldnodes:
+                if oldnode in moves:
+                    continue
+                if len(newnodes) > 1:
+                    # usually a split, take the one with biggest rev number
+                    newnode = next(unfi.set('max(%ln)', newnodes)).node()
+                elif len(newnodes) == 0:
+                    # move bookmark backwards
+                    allreplaced = []
+                    for rep in replacements:
+                        allreplaced.extend(rep)
+                    roots = list(unfi.set('max((::%n) - %ln)', oldnode,
+                                          allreplaced))
+                    if roots:
+                        newnode = roots[0].node()
+                    else:
+                        newnode = nullid
                 else:
-                    newnode = nullid
-            else:
-                newnode = newnodes[0]
-            moves[oldnode] = newnode
+                    newnode = newnodes[0]
+                moves[oldnode] = newnode
 
     allnewnodes = [n for ns in replacements.values() for n in ns]
     toretract = {}



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