D3818: scmutil: make cleanupnodes optionally also fix the phase

[yuja (Yuya Nishihara)]

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