[PATCH 4 of 5] phases: calculate the maxphase and minphaserev

[Pierre-Yves David]

On 10/09/2014 12:22 PM, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham at fb.com>
> # Date 1412707606 25200
> #      Tue Oct 07 11:46:46 2014 -0700
> # Node ID 80710688335b3e647bbf74700cb6e1a464b909b2
> # Parent  9291d6a89c9b9bf93cbb920c98795fa13fbd6fa0
> phases: calculate the maxphase and minphaserev

Patch idea looks fine. But I've a couple of technical question.

 From a patchbombing perspective it would have been nice to group it 
with something actually using all the new concept. (for example, could 
have been swaped with the next ones.)

> This calculates and stores the maxphase in the repo, and the minimum phased rev.
> These will be used in future patches to optimize the phase computation. It's
> already used in this patch to optimize returning public (0) for revs below the
> minimum phased rev.
>
> diff --git a/mercurial/phases.py b/mercurial/phases.py
> --- a/mercurial/phases.py
> +++ b/mercurial/phases.py
> @@ -146,6 +146,10 @@ class phasecache(object):
>               # Cheap trick to allow shallow-copy without copy module
>               self.phaseroots, self.dirty = _readroots(repo, phasedefaults)
>               self._phaserevs = None
> +            # The maximum phase in this repository.
> +            self._maxphase = 0
> +            # The minimum rev in the repo that is not phase 0.
> +            self._minphaserev = 0

Looks like you are using "0" as "not-initialized value" while 0 is a 
valid value in both case. Could we use None here instead?

>               self.filterunknown(repo)
>               self.opener = repo.sopener
>
> @@ -157,10 +161,14 @@ class phasecache(object):
>           ph.dirty = self.dirty
>           ph.opener = self.opener
>           ph._phaserevs = self._phaserevs
> +        ph._maxphase = self._maxphase
> +        ph._minphaserev = self._minphaserev
>           return ph
>
>       def replace(self, phcache):
> -        for a in 'phaseroots dirty opener _phaserevs'.split():
> +        attributes = ['phaseroots', 'dirty', 'opener', '_phaserevs',
> +            '_maxphase', '_minphaserev']

bad indentation. should be

 > +        attributes = ['phaseroots', 'dirty', 'opener', '_phaserevs',
 > +                      '_maxphase', '_minphaserev']


> +        for a in attributes:
>               setattr(self, a, getattr(phcache, a))
>
>       def getphaserevs(self, repo):
> @@ -180,14 +188,19 @@ class phasecache(object):
>
>       def invalidate(self):
>           self._phaserevs = None
> +        self._maxphase = 0
> +        self._minphaserev = 0

same feedback about using 0 as default value.

>
>       def _populatephaseroots(self, repo):
>           """Fills the _phaserevs cache with phases for the roots.
>           """
> +        self._minphaserev = len(repo)
>           cl = repo.changelog
>           phaserevs = self._phaserevs
>           for phase in trackedphases:
>               roots = map(cl.rev, self.phaseroots[phase])
> +            if roots:
> +                self._maxphase = max(self._maxphase, phase)
>               for root in roots:
>                   phaserevs[root] = phase
>                   # We know that the parent of a root has a phase less than
> @@ -196,6 +209,7 @@ class phasecache(object):
>                       for parent in cl.parentrevs(root):
>                           if parent != -1:
>                               phaserevs[parent] = 0
> +                self._minphaserev = min(self._minphaserev, root)
>
>       def phase(self, repo, rev):
>           # We need a repo argument here to be able to build _phaserevs
> @@ -207,6 +221,8 @@ class phasecache(object):
>               return public
>           if rev < nullrev:
>               raise ValueError(_('cannot lookup negative revision'))
> +        if rev < self._minphaserev or rev <= nullrev:
> +            return 0
>           if self._phaserevs is None or rev >= len(self._phaserevs):
>               self.invalidate()
>               self._phaserevs = self.getphaserevs(repo)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>

-- 
Pierre-Yves David