D3818: scmutil: make cleanupnodes optionally also fix the phase

[martinvonz (Martin von Zweigbergk)]

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