[PATCH 5 of 5] phases: retractboundary should update all affected phases

[Pierre-Yves David]

On 10/09/2014 12:22 PM, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham at fb.com>
> # Date 1412707733 25200
> #      Tue Oct 07 11:48:53 2014 -0700
> # Node ID be5f4f8f38440da11821946ae84abfb199190656
> # Parent  80710688335b3e647bbf74700cb6e1a464b909b2
> phases: retractboundary should update all affected phases

Patch looks good, should come before patch 4. Couple of minor feedback 
mostly request for more doc and comment since I got me some time to 
figure out if this patch was correct.

>
> Previously retractboundary only updated the roots for the phase it was changing.
> This meant there might be other roots in other phases that were now incorrect.
> It just happened to work fine, because the old algorithms allowed future phases
> to overwrite the results of earlier phases.  The upcoming new phase algorithm
> requires the roots to be correct, so this patch fixes up all roots affected by
> the boundary retracting.

We could probably use an actual example of such cache in the description.

>
> diff --git a/mercurial/phases.py b/mercurial/phases.py
> --- a/mercurial/phases.py
> +++ b/mercurial/phases.py
> @@ -287,7 +287,18 @@ class phasecache(object):
>                   raise util.Abort(_('cannot change null revision phase'))
>               currentroots = currentroots.copy()
>               currentroots.update(newroots)
> -            ctxs = repo.set('roots(%ln::)', currentroots)
> +            affectedrevs = repo.revs('%ln::', currentroots)
> +            for phase, roots in enumerate(self.phaseroots):
> +                if targetphase <= phase:
> +                    continue
> +                roots = roots.copy()
> +                for rev in affectedrevs:
> +                    node = repo.changelog.node(rev)

You are apparently recomputing the rev → node mapping for every 
iteration of the first look. Can we can cache it instead?

As we are at it. testing the other way around (iterating over roots) may 
be faster for huge phases movement.

> +                    if node in roots:
> +                        roots.discard(node)
> +                self._updateroots(phase, roots, tr)

Probably worth adding a comment so that people can figure out what this 
is about in the future.

> +
> +            ctxs = repo.set('roots(%ld)', affectedrevs)
>               currentroots.intersection_update(ctx.node() for ctx in ctxs)
>               self._updateroots(targetphase, currentroots, tr)
>           repo.invalidatevolatilesets()


-- 
Pierre-Yves David