Patchwork D3818: scmutil: make cleanupnodes optionally also fix the phase

login
register
mail settings
Submitter phabricator
Date June 19, 2018, 9:17 p.m.
Message ID <differential-rev-PHID-DREV-txqwfv3qenrl3k7ztmfx-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/32339/
State Superseded
Headers show

Comments

phabricator - June 19, 2018, 9:17 p.m.
martinvonz created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This also updates all applicable callers so we get some testing of it.
  
  Note that rebase and histedit can't be fixed yet because they call
  cleanupnodes() only at the end and the phase may have been changed by
  the user when the rebase/histedit was interrupted (due to merge
  conflicts).
  
  Also not that amend can't rely on cleanupnodes, because it allows the
  phase to be overridden on the command line (using `hg amend
  --secret`).

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  contrib/phabricator.py
  hgext/fix.py
  hgext/uncommit.py
  mercurial/cmdutil.py
  mercurial/scmutil.py

CHANGE DETAILS




To: martinvonz, #hg-reviewers
Cc: mercurial-devel
phabricator - June 19, 2018, 9:20 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> phabricator.py:593
>                               date=None, local=True)
> -            scmutil.cleanupnodes(repo, mapping, b'phabsend')
> +            scmutil.cleanupnodes(repo, mapping, b'phabsend', fixphase=True)
>              if wnode in mapping:

This line means that if you use phabricator from your hg repo, you'll need to remember to run `./hg phabsend` (otherwise it will crash when trying to add URL in the commit message and you may end up with duplicate reviews like I did)

REPOSITORY
  rHG Mercurial

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

To: martinvonz, #hg-reviewers
Cc: mercurial-devel
Yuya Nishihara - June 20, 2018, 1:43 p.m.
> +def cleanupnodes(repo, replacements, operation, moves=None, metadata=None,
> +                 fixphase=False):
>      """do common cleanups when old nodes are replaced by new nodes
>  
>      That includes writing obsmarkers or stripping nodes, and moving bookmarks.
> @@ -825,11 +826,34 @@
>              newnode = newnodes[0]
>          moves[oldnode] = newnode
>  
> +    allnewnodes = [n for ns in replacements.values() for n in ns]
> +    toretract = {}
> +    toadvance = {}
> +    if fixphase:
> +        precursors = {}
> +        for oldnode, newnodes in replacements.items():
> +            for newnode in newnodes:
> +                precursors.setdefault(newnode, []).append(oldnode)
> +
> +        allnewnodes.sort(key=lambda n: repo[n].rev())
> +        newphase = {}
> +        def phase(ctx):
> +            return newphase.get(ctx.node(), ctx.phase())
> +        for newnode in allnewnodes:
> +            ctx = repo[newnode]
> +            oldphase = max(repo[oldnode].phase()
> +                           for oldnode in precursors[newnode])
> +            parentphase = max(phase(p) for p in ctx.parents())
> +            newphase[newnode] = max(oldphase, parentphase)
> +            if newphase[newnode] > ctx.phase():
> +                toretract.setdefault(newphase[newnode], []).append(newnode)
> +            elif newphase[newnode] < ctx.phase():
> +                toadvance.setdefault(newphase[newnode], []).append(newnode)
> +
>      with repo.transaction('cleanup') as tr:
>          # Move bookmarks
>          bmarks = repo._bookmarks
>          bmarkchanges = []
> -        allnewnodes = [n for ns in replacements.values() for n in ns]
>          for oldnode, newnode in moves.items():
>              oldbmarks = repo.nodebookmarks(oldnode)
>              if not oldbmarks:
> @@ -850,6 +874,11 @@
>          if bmarkchanges:
>              bmarks.applychanges(repo, tr, bmarkchanges)
>  
> +        for phase, nodes in toretract.items():
> +            phases.retractboundary(repo, tr, phase, nodes)
> +        for phase, nodes in toadvance.items():
> +            phases.advanceboundary(repo, tr, phase, nodes)

The logic looks good to me, but I'm not sure if recomputing a proper phase
move here is better than the original config override.

> --- a/hgext/fix.py
> +++ b/hgext/fix.py
> @@ -158,7 +158,7 @@
>                  del filedata[rev]
>  
>          replacements = {prec: [succ] for prec, succ in replacements.iteritems()}
> -        scmutil.cleanupnodes(repo, replacements, 'fix')
> +        scmutil.cleanupnodes(repo, replacements, 'fix', fixphase=True)

Perhaps this has to be wrapped by a transaction. Otherwise, I think a secret
changeset could be temporarily exposed until cleanupnodes().
phabricator - June 20, 2018, 1:44 p.m.
yuja added a comment.


  > +def cleanupnodes(repo, replacements, operation, moves=None, metadata=None,
  >  +                 fixphase=False):
  > 
  >   """do common cleanups when old nodes are replaced by new nodes
  >     
  >   That includes writing obsmarkers or stripping nodes, and moving bookmarks.
  > 
  > @@ -825,11 +826,34 @@
  > 
  >       newnode = newnodes[0]
  >   moves[oldnode] = newnode
  >     
  > 
  > +    allnewnodes = [n for ns in replacements.values() for n in ns]
  >  +    toretract = {}
  >  +    toadvance = {}
  >  +    if fixphase:
  >  +        precursors = {}
  >  +        for oldnode, newnodes in replacements.items():
  >  +            for newnode in newnodes:
  >  +                precursors.setdefault(newnode, []).append(oldnode)
  >  +
  >  +        allnewnodes.sort(key=lambda n: repo[n].rev())
  >  +        newphase = {}
  >  +        def phase(ctx):
  >  +            return newphase.get(ctx.node(), ctx.phase())
  >  +        for newnode in allnewnodes:
  >  +            ctx = repo[newnode]
  >  +            oldphase = max(repo[oldnode].phase()
  >  +                           for oldnode in precursors[newnode])
  >  +            parentphase = max(phase(p) for p in ctx.parents())
  >  +            newphase[newnode] = max(oldphase, parentphase)
  >  +            if newphase[newnode] > ctx.phase():
  >  +                toretract.setdefault(newphase[newnode], []).append(newnode)
  >  +            elif newphase[newnode] < ctx.phase():
  >  +                toadvance.setdefault(newphase[newnode], []).append(newnode)
  >  +
  > 
  >   with repo.transaction('cleanup') as tr:
  >       # Move bookmarks
  >       bmarks = repo._bookmarks
  >       bmarkchanges = []
  > 
  > - allnewnodes = [n for ns in replacements.values() for n in ns] for oldnode, newnode in moves.items(): oldbmarks = repo.nodebookmarks(oldnode) if not oldbmarks: @@ -850,6 +874,11 @@ if bmarkchanges: bmarks.applychanges(repo, tr, bmarkchanges)
  > 
  >   +        for phase, nodes in toretract.items(): +            phases.retractboundary(repo, tr, phase, nodes) +        for phase, nodes in toadvance.items(): +            phases.advanceboundary(repo, tr, phase, nodes)
  
  The logic looks good to me, but I'm not sure if recomputing a proper phase
  move here is better than the original config override.
  
  >   - a/hgext/fix.py +++ b/hgext/fix.py @@ -158,7 +158,7 @@ del filedata[rev]
  > 
  >     replacements = {prec: [succ] for prec, succ in replacements.iteritems()}
  > - scmutil.cleanupnodes(repo, replacements, 'fix') +        scmutil.cleanupnodes(repo, replacements, 'fix', fixphase=True)
  
  Perhaps this has to be wrapped by a transaction. Otherwise, I think a secret
  changeset could be temporarily exposed until cleanupnodes().

REPOSITORY
  rHG Mercurial

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

To: martinvonz, #hg-reviewers
Cc: yuja, mercurial-devel
phabricator - June 20, 2018, 4:29 p.m.
mharbison72 added a comment.


  I assume this was inspired at least in part by https://bz.mercurial-scm.org/show_bug.cgi?id=5918?
  
  When I was considering how to do this, I was thinking pass in an explicit phase to handle `hg amend --secret` and similar cases, and `None` if the phase should just be inherited from the old node(s).  Are there cases where nodes are replaced, but you don't want to change the phase at all?

REPOSITORY
  rHG Mercurial

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

To: martinvonz, #hg-reviewers
Cc: mharbison72, yuja, mercurial-devel
phabricator - June 20, 2018, 4:49 p.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/D3818#59697, @mharbison72 wrote:
  
  > I assume this was inspired at least in part by https://bz.mercurial-scm.org/show_bug.cgi?id=5918?
  
  
  It was inspired by https://phab.mercurial-scm.org/D2016, but I had also seen your bug report.
  
  > When I was considering how to do this, I was thinking pass in an explicit phase to handle `hg amend --secret` and similar cases, and `None` if the phase should just be inherited from the old node(s).
  
  Oh, I had not seen that you mentioned cleanupnodes() in that bug (or I did and forgot) :) That sounds like a good idea.
  
  >   Are there cases where nodes are replaced, but you don't want to change the phase at all?
  
  Yes, unfortunately: rebase and histedit (as mentioned in commit message).
  
  When I originally started working on this right after Kyle sent https://phab.mercurial-scm.org/D2016, I wanted to first fix rebase and histedit so they made the changes incrementally and we only ever had to call cleanupnodes() with one precursor. See comments on https://phab.mercurial-scm.org/D2013 for my plans. It turned out to be (more work than I had hoped)^2 (i.e. more than I had hoped after I figured out it was more than I had hoped). The issue with making rebase and histedit create obsmarkers and move bookmarks incrementally is that we need to keep track of those things so we can undo them on --abort. Then I started thinking that maybe the first step towards that goal would be to add a generic `hg undo`.
  
  Anyway, I like the idea of being able to use cleanupnodes() for amend as well. Perhaps something like this:
  
  fixphase=False, targetphase=None => leave phases alone (used by rebase and histedit)
  fixphase=True, targetphase=None => set phases based just on the precursors' phase (used by most other commands)
  fixphase=True, targetphase=<phase> => set phase to targetphase unless parent has higher phase (used by `hg amend --secret`)
  fixphase=False, targetphase=<phase> => assertion error

REPOSITORY
  rHG Mercurial

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

To: martinvonz, #hg-reviewers
Cc: mharbison72, yuja, mercurial-devel
phabricator - June 20, 2018, 5 p.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/D3818#59695, @yuja wrote:
  
  > The logic looks good to me, but I'm not sure if recomputing a proper phase
  >  move here is better than the original config override.
  
  
  The goal is not simplification, but to reduce the risk of bugs. We've had many bugs in this area: phabricator, fix, split, evolve, and others. If we just point extension writers to cleanupnodes(), it seems less likely that they'll make the mistake. Especially in a cycle or two when we've made fixphase=True the default. I'll update the commit message, since I admit that justification was completely missing.
  
  > 
  > 
  >>   - a/hgext/fix.py +++ b/hgext/fix.py @@ -158,7 +158,7 @@ del filedata[rev]
  >> 
  >>     replacements = {prec: [succ] for prec, succ in replacements.iteritems()}
  >> - scmutil.cleanupnodes(repo, replacements, 'fix') +        scmutil.cleanupnodes(repo, replacements, 'fix', fixphase=True)
  > 
  > Perhaps this has to be wrapped by a transaction. Otherwise, I think a secret
  >  changeset could be temporarily exposed until cleanupnodes().
  
  Good point. Addressed in https://phab.mercurial-scm.org/D3823.

REPOSITORY
  rHG Mercurial

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

To: martinvonz, #hg-reviewers
Cc: mharbison72, yuja, mercurial-devel
phabricator - June 21, 2018, 2:31 a.m.
mharbison72 added a comment.


  In https://phab.mercurial-scm.org/D3818#59706, @martinvonz wrote:
  
  > Anyway, I like the idea of being able to use cleanupnodes() for amend as well. Perhaps something like this:
  >
  > fixphase=False, targetphase=None => leave phases alone (used by rebase and histedit)
  >  fixphase=True, targetphase=None => set phases based just on the precursors' phase (used by most other commands)
  >  fixphase=True, targetphase=<phase> => set phase to targetphase unless parent has higher phase (used by `hg amend --secret`)
  >  fixphase=False, targetphase=<phase> => assertion error
  
  
  It always bugs me when an API can allow a bad combination of arguments, but I don't see a better way, and I think the consistent use of cleanupnodes() is more important.

REPOSITORY
  rHG Mercurial

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

To: martinvonz, #hg-reviewers
Cc: mharbison72, yuja, mercurial-devel
phabricator - June 21, 2018, 5:44 a.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/D3818#59714, @mharbison72 wrote:
  
  > In https://phab.mercurial-scm.org/D3818#59706, @martinvonz wrote:
  >
  > > Anyway, I like the idea of being able to use cleanupnodes() for amend as well. Perhaps something like this:
  > >
  > > fixphase=False, targetphase=None => leave phases alone (used by rebase and histedit)
  > >  fixphase=True, targetphase=None => set phases based just on the precursors' phase (used by most other commands)
  > >  fixphase=True, targetphase=<phase> => set phase to targetphase unless parent has higher phase (used by `hg amend --secret`)
  > >  fixphase=False, targetphase=<phase> => assertion error
  >
  >
  > It always bugs me when an API can allow a bad combination of arguments, but I don't see a better way, and I think the consistent use of cleanupnodes() is more important.
  
  
  I agree. It's also unfortunate to take up two arguments for what should be one. After a few years of Python, I've still not quite gotten used to arguments of mixed types, but perhaps {None, 'auto', <integer phase>} would be better?

REPOSITORY
  rHG Mercurial

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

To: martinvonz, #hg-reviewers
Cc: mharbison72, yuja, mercurial-devel
phabricator - June 21, 2018, 6:40 a.m.
quark added a comment.


  Not directly related to this patch. On API complexity: One of the unimplemented ideas is to require a transaction and make `operation` optional - default to the transaction name.

REPOSITORY
  rHG Mercurial

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

To: martinvonz, #hg-reviewers
Cc: quark, mharbison72, yuja, mercurial-devel
phabricator - June 21, 2018, 6:46 a.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/D3818#59716, @quark wrote:
  
  > Not directly related to this patch. On API complexity: One of the unimplemented ideas is to require a transaction and make `operation` optional - default to the transaction name.
  
  
  Good idea. I like passing around the transaction explicitly.
  
  I also hope to get rid of the "moves" argument once we have made rebase and histedit add obsmarkers and move bookmarks incrementally.

REPOSITORY
  rHG Mercurial

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

To: martinvonz, #hg-reviewers
Cc: quark, mharbison72, yuja, mercurial-devel
Yuya Nishihara - June 21, 2018, 12:37 p.m.
>   > Not directly related to this patch. On API complexity: One of the unimplemented ideas is to require a transaction and make `operation` optional - default to the transaction name.
>   
>   
>   Good idea. I like passing around the transaction explicitly.

+1. An explicit `tr` argument will make sure to wrap cleanupnodes() with
transaction.
phabricator - June 21, 2018, 12:43 p.m.
yuja added a comment.


  >   > Not directly related to this patch. On API complexity: One of the unimplemented ideas is to require a transaction and make `operation` optional - default to the transaction name.
  >   
  >   
  >   Good idea. I like passing around the transaction explicitly.
  
  +1. An explicit `tr` argument will make sure to wrap cleanupnodes() with
  transaction.

REPOSITORY
  rHG Mercurial

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

To: martinvonz, #hg-reviewers
Cc: quark, mharbison72, yuja, mercurial-devel
phabricator - June 21, 2018, 3:15 p.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/D3818#59722, @yuja wrote:
  
  > Queued, thanks.
  >
  > > +            ctx = unfi[newnode]
  > >  +            if targetphase is None:
  > >  +                oldphase = max(unfi[oldnode].phase()
  > >  +                               for oldnode in precursors[newnode])
  > >  +                parentphase = max(phase(p) for p in ctx.parents())
  > >  +                newphase = max(oldphase, parentphase)
  > >  +            else:
  > >  +                newphase = targetphase
  >
  > Maybe better to do `max(targetphase, parentphase)`?
  >
  > I think it can be a source of bug that the targetphase may advance ancestors
  >  to public/draft.
  
  
  Oh, I definitely agree. Good catch. Will send a followup.

REPOSITORY
  rHG Mercurial

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

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

Patch

diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
--- a/mercurial/scmutil.py
+++ b/mercurial/scmutil.py
@@ -779,7 +779,8 @@ 
     def __contains__(self, node):
         return self._revcontains(self._torev(node))
 
-def cleanupnodes(repo, replacements, operation, moves=None, metadata=None):
+def cleanupnodes(repo, replacements, operation, moves=None, metadata=None,
+                 fixphase=False):
     """do common cleanups when old nodes are replaced by new nodes
 
     That includes writing obsmarkers or stripping nodes, and moving bookmarks.
@@ -825,11 +826,34 @@ 
             newnode = newnodes[0]
         moves[oldnode] = newnode
 
+    allnewnodes = [n for ns in replacements.values() for n in ns]
+    toretract = {}
+    toadvance = {}
+    if fixphase:
+        precursors = {}
+        for oldnode, newnodes in replacements.items():
+            for newnode in newnodes:
+                precursors.setdefault(newnode, []).append(oldnode)
+
+        allnewnodes.sort(key=lambda n: repo[n].rev())
+        newphase = {}
+        def phase(ctx):
+            return newphase.get(ctx.node(), ctx.phase())
+        for newnode in allnewnodes:
+            ctx = repo[newnode]
+            oldphase = max(repo[oldnode].phase()
+                           for oldnode in precursors[newnode])
+            parentphase = max(phase(p) for p in ctx.parents())
+            newphase[newnode] = max(oldphase, parentphase)
+            if newphase[newnode] > ctx.phase():
+                toretract.setdefault(newphase[newnode], []).append(newnode)
+            elif newphase[newnode] < ctx.phase():
+                toadvance.setdefault(newphase[newnode], []).append(newnode)
+
     with repo.transaction('cleanup') as tr:
         # Move bookmarks
         bmarks = repo._bookmarks
         bmarkchanges = []
-        allnewnodes = [n for ns in replacements.values() for n in ns]
         for oldnode, newnode in moves.items():
             oldbmarks = repo.nodebookmarks(oldnode)
             if not oldbmarks:
@@ -850,6 +874,11 @@ 
         if bmarkchanges:
             bmarks.applychanges(repo, tr, bmarkchanges)
 
+        for phase, nodes in toretract.items():
+            phases.retractboundary(repo, tr, phase, nodes)
+        for phase, nodes in toadvance.items():
+            phases.advanceboundary(repo, tr, phase, nodes)
+
         # Obsolete or strip nodes
         if obsolete.isenabled(repo, obsolete.createmarkersopt):
             # If a node is already obsoleted, and we want to obsolete it
diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -784,16 +784,12 @@ 
                                 extra=extra,
                                 branch=label)
 
-            commitphase = ctx.phase()
-            overrides = {('phases', 'new-commit'): commitphase}
-            with repo.ui.configoverride(overrides, 'branch-change'):
-                newnode = repo.commitctx(mc)
-
+            newnode = repo.commitctx(mc)
             replacements[ctx.node()] = (newnode,)
             ui.debug('new node id is %s\n' % hex(newnode))
 
         # create obsmarkers and move bookmarks
-        scmutil.cleanupnodes(repo, replacements, 'branch-change')
+        scmutil.cleanupnodes(repo, replacements, 'branch-change', fixphase=True)
 
         # move the working copy too
         wctx = repo[None]
diff --git a/hgext/uncommit.py b/hgext/uncommit.py
--- a/hgext/uncommit.py
+++ b/hgext/uncommit.py
@@ -91,12 +91,7 @@ 
                          user=ctx.user(),
                          date=ctx.date(),
                          extra=ctx.extra())
-    # phase handling
-    commitphase = ctx.phase()
-    overrides = {('phases', 'new-commit'): commitphase}
-    with repo.ui.configoverride(overrides, 'uncommit'):
-        newid = repo.commitctx(new)
-    return newid
+    return repo.commitctx(new)
 
 def _fixdirstate(repo, oldctx, newctx, status):
     """ fix the dirstate after switching the working directory from oldctx to
@@ -183,7 +178,7 @@ 
                 # Fully removed the old commit
                 mapping[old.node()] = ()
 
-            scmutil.cleanupnodes(repo, mapping, 'uncommit')
+            scmutil.cleanupnodes(repo, mapping, 'uncommit', fixphase=True)
 
             with repo.dirstate.parentchange():
                 repo.dirstate.setparents(newid, node.nullid)
@@ -242,12 +237,7 @@ 
                                 user=predctx.user(),
                                 date=predctx.date(),
                                 extra=extras)
-        # phase handling
-        commitphase = curctx.phase()
-        overrides = {('phases', 'new-commit'): commitphase}
-        with repo.ui.configoverride(overrides, 'uncommit'):
-            newprednode = repo.commitctx(newctx)
-
+        newprednode = repo.commitctx(newctx)
         newpredctx = repo[newprednode]
         dirstate = repo.dirstate
 
@@ -257,4 +247,4 @@ 
             _fixdirstate(repo, curctx, newpredctx, s)
 
         mapping = {curctx.node(): (newprednode,)}
-        scmutil.cleanupnodes(repo, mapping, 'unamend')
+        scmutil.cleanupnodes(repo, mapping, 'unamend', fixphase=True)
diff --git a/hgext/fix.py b/hgext/fix.py
--- a/hgext/fix.py
+++ b/hgext/fix.py
@@ -158,7 +158,7 @@ 
                 del filedata[rev]
 
         replacements = {prec: [succ] for prec, succ in replacements.iteritems()}
-        scmutil.cleanupnodes(repo, replacements, 'fix')
+        scmutil.cleanupnodes(repo, replacements, 'fix', fixphase=True)
 
 def getworkqueue(ui, repo, pats, opts, revstofix, basectxs):
     """"Constructs the list of files to be fixed at specific revisions
@@ -484,25 +484,23 @@ 
             isexec=fctx.isexec(),
             copied=copied)
 
-    overrides = {('phases', 'new-commit'): ctx.phase()}
-    with ui.configoverride(overrides, source='fix'):
-        memctx = context.memctx(
-            repo,
-            parents=(newp1node, newp2node),
-            text=ctx.description(),
-            files=set(ctx.files()) | set(filedata.keys()),
-            filectxfn=filectxfn,
-            user=ctx.user(),
-            date=ctx.date(),
-            extra=ctx.extra(),
-            branch=ctx.branch(),
-            editor=None)
-        sucnode = memctx.commit()
-        prenode = ctx.node()
-        if prenode == sucnode:
-            ui.debug('node %s already existed\n' % (ctx.hex()))
-        else:
-            replacements[ctx.node()] = sucnode
+    memctx = context.memctx(
+        repo,
+        parents=(newp1node, newp2node),
+        text=ctx.description(),
+        files=set(ctx.files()) | set(filedata.keys()),
+        filectxfn=filectxfn,
+        user=ctx.user(),
+        date=ctx.date(),
+        extra=ctx.extra(),
+        branch=ctx.branch(),
+        editor=None)
+    sucnode = memctx.commit()
+    prenode = ctx.node()
+    if prenode == sucnode:
+        ui.debug('node %s already existed\n' % (ctx.hex()))
+    else:
+        replacements[ctx.node()] = sucnode
 
 def getfixers(ui):
     """Returns a map of configured fixer tools indexed by their names
diff --git a/contrib/phabricator.py b/contrib/phabricator.py
--- a/contrib/phabricator.py
+++ b/contrib/phabricator.py
@@ -580,9 +580,7 @@ 
                         repo, old, parents=parents, text=newdesc,
                         user=old.user(), date=old.date(), extra=old.extra())
 
-                    overrides = {(b'phases', b'new-commit'): old.phase()}
-                    with ui.configoverride(overrides, b'phabsend'):
-                        newnode = new.commit()
+                    newnode = new.commit()
 
                     mapping[old.node()] = [newnode]
                     # Update diff property
@@ -592,7 +590,7 @@ 
                 if tagname in repo.tags():
                     tags.tag(repo, tagname, nullid, message=None, user=None,
                              date=None, local=True)
-            scmutil.cleanupnodes(repo, mapping, b'phabsend')
+            scmutil.cleanupnodes(repo, mapping, b'phabsend', fixphase=True)
             if wnode in mapping:
                 unfi.setparents(mapping[wnode][0])