Patchwork D6880: rebase: track new nodes when --keep is set

login
register
mail settings
Submitter phabricator
Date Sept. 25, 2019, 3:35 p.m.
Message ID <differential-rev-PHID-DREV-z6olvzrkgbd52svx53v5-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/41758/
State Superseded
Headers show

Comments

phabricator - Sept. 25, 2019, 3:35 p.m.
pgossman created this revision.
Herald added a reviewer: martinvonz.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  When --keep is passed with rebase, the new nodes created are not
  accessible from templates.
  
  This change enables accessing the newly-created nodes from nodechanges,
  just as if --keep was not set.

REPOSITORY
  rHG Mercurial

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

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

CHANGE DETAILS




To: pgossman, martinvonz, #hg-reviewers
Cc: mercurial-devel
phabricator - Sept. 26, 2019, 11:20 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> rebase.py:1808-1810
> +    if keepf:
> +        replacements = {}
> +    scmutil.cleanupnodes(repo, replacements, 'rebase', moves, backup=backup)

It looks like this still moves bookmarks, right (in the `moves` dict)? Should `scmutil.cleanupnodes()` simply only be called if `not keepf`?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6880/new/

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

To: pgossman, martinvonz, #hg-reviewers, pulkit
Cc: mercurial-devel
phabricator - Sept. 26, 2019, 11:29 p.m.
pgossman added inline comments.

INLINE COMMENTS

> martinvonz wrote in rebase.py:1808-1810
> It looks like this still moves bookmarks, right (in the `moves` dict)? Should `scmutil.cleanupnodes()` simply only be called if `not keepf`?

Yes this moves them. With --keep, bookmarks should still be moved.

See rebase help or https://bz.mercurial-scm.org/show_bug.cgi?id=5682

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6880/new/

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

To: pgossman, martinvonz, #hg-reviewers, pulkit
Cc: mercurial-devel
phabricator - Sept. 27, 2019, 12:16 a.m.
martinvonz added inline comments.

INLINE COMMENTS

> pgossman wrote in rebase.py:1808-1810
> Yes this moves them. With --keep, bookmarks should still be moved.
> 
> See rebase help or https://bz.mercurial-scm.org/show_bug.cgi?id=5682

Oh, right, that bug... I agree that it's good that this patch preserves that behavior to avoid doing multiple things in one patch. But I think the current behavior is pretty surprising. I'm curious what you think since you apparently use `hg rebase --keep` (I don't).

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6880/new/

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

To: pgossman, martinvonz, #hg-reviewers, pulkit
Cc: mercurial-devel
phabricator - Sept. 30, 2019, 4:23 p.m.
pgossman added inline comments.

INLINE COMMENTS

> martinvonz wrote in rebase.py:1808-1810
> Oh, right, that bug... I agree that it's good that this patch preserves that behavior to avoid doing multiple things in one patch. But I think the current behavior is pretty surprising. I'm curious what you think since you apparently use `hg rebase --keep` (I don't).

Bookmarks don't affect my use case for `--keep`, however, I agree the behavior was surprising and it would seem more intuitive if `--keep` did not move bookmarks.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6880/new/

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

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

Patch

diff --git a/tests/test-rebase-templates.t b/tests/test-rebase-templates.t
--- a/tests/test-rebase-templates.t
+++ b/tests/test-rebase-templates.t
@@ -53,5 +53,32 @@ 
   o  0:18d04c59bb5d Added a
   
 
-  $ hg rebase -s 6 -d 4 -q -T "{nodechanges % '{oldnode}:{newnodes % ' {node} '}'}"
-  d9d6773efc831c274eace04bc13e8e6412517139: f48cd65c6dc3d2acb55da54402a5b029546e546f  (no-eol)
+  $ hg rebase -s 6 -d 4 -q -T "{nodechanges % '{oldnode}:{newnodes % ' {node} '}'}\n"
+  d9d6773efc831c274eace04bc13e8e6412517139: f48cd65c6dc3d2acb55da54402a5b029546e546f 
+
+  $ hg log -G -T "{rev}:{node|short} {desc}"
+  o  7:f48cd65c6dc3 Added b
+  |
+  | @  5:df21b32134ba Added d
+  |/
+  o  4:849767420fd5 Added c
+  |
+  o  0:18d04c59bb5d Added a
+  
+
+
+  $ hg rebase -s 7 -d 5 -q --keep -T "{nodechanges % '{oldnode}:{newnodes % ' {node} '}'}\n"
+  f48cd65c6dc3d2acb55da54402a5b029546e546f: 6f7dda91e55e728fb798f3e44dbecf0ebaa83267 
+
+  $ hg log -G -T "{rev}:{node|short} {desc}"
+  o  8:6f7dda91e55e Added b
+  |
+  | o  7:f48cd65c6dc3 Added b
+  | |
+  @ |  5:df21b32134ba Added d
+  |/
+  o  4:849767420fd5 Added c
+  |
+  o  0:18d04c59bb5d Added a
+  
+
diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -1783,20 +1783,18 @@ 
             oldnode = tonode(rev)
             newnode = collapsedas or tonode(newrev)
             moves[oldnode] = newnode
-            if not keepf:
-                succs = None
-                if rev in skipped:
-                    if stripcleanup or not repo[rev].obsolete():
-                        succs = ()
-                elif collapsedas:
-                    collapsednodes.append(oldnode)
-                else:
-                    succs = (newnode,)
-                if succs is not None:
-                    replacements[(oldnode,)] = succs
+            succs = None
+            if rev in skipped:
+                if stripcleanup or not repo[rev].obsolete():
+                    succs = ()
+            elif collapsedas:
+                collapsednodes.append(oldnode)
+            else:
+                succs = (newnode,)
+            if succs is not None:
+                replacements[(oldnode,)] = succs
     if collapsednodes:
         replacements[tuple(collapsednodes)] = (collapsedas,)
-    scmutil.cleanupnodes(repo, replacements, 'rebase', moves, backup=backup)
     if fm:
         hf = fm.hexfunc
         fl = fm.formatlist
@@ -1807,6 +1805,9 @@ 
                 changes[hf(oldn)] = fl([hf(n) for n in newn], name='node')
         nodechanges = fd(changes, key="oldnode", value="newnodes")
         fm.data(nodechanges=nodechanges)
+    if keepf:
+        replacements = {}
+    scmutil.cleanupnodes(repo, replacements, 'rebase', moves, backup=backup)
 
 def pullrebase(orig, ui, repo, *args, **opts):
     'Call rebase after pull if the latter has been invoked with --rebase'