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
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
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
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
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'