[PATCH 3 of 5] phases: move root phase assignment to it's own function

[Pierre-Yves David]

On 10/13/2014 10:59 AM, Durham Goode wrote:
>
> On 10/12/14 11:33 PM, Pierre-Yves David wrote:
>>
>>
>> On 10/09/2014 12:22 PM, Durham Goode wrote:
>>> # HG changeset patch
>>> # User Durham Goode <durham at fb.com>
>>> # Date 1412707357 25200
>>> #      Tue Oct 07 11:42:37 2014 -0700
>>> # Node ID 9291d6a89c9b9bf93cbb920c98795fa13fbd6fa0
>>> # Parent  34e934d15e8c9a8c5809dc1909cbdfcba1017603
>>> phases: move root phase assignment to it's own function
>>>
>>> This moves the initial root phase assignment to it's own function.
>>> Future
>>> patches which make phase calculations lazy will use this function to
>>> pre-fill
>>> certain phases which can be deduced from the roots.
>>>
>>> diff --git a/mercurial/phases.py b/mercurial/phases.py
>>> --- a/mercurial/phases.py
>>> +++ b/mercurial/phases.py
>>> @@ -167,6 +167,8 @@ class phasecache(object):
>>>           if self._phaserevs is None:
>>>               repo = repo.unfiltered()
>>>               revs = [public] * len(repo.changelog)
>>> +            self._phaserevs = revs
>>> +            self._populatephaseroots(repo)
>>>               for phase in trackedphases:
>>>                   roots = map(repo.changelog.rev,
>>> self.phaseroots[phase])
>>>                   if roots:
>>> @@ -174,11 +176,27 @@ class phasecache(object):
>>>                           revs[rev] = phase
>>>                       for rev in repo.changelog.descendants(roots):
>>>                           revs[rev] = phase
>>> -            self._phaserevs = revs
>>>           return self._phaserevs
>>> +
>>>       def invalidate(self):
>>>           self._phaserevs = None
>>>
>>> +    def _populatephaseroots(self, repo):
>>> +        """Fills the _phaserevs cache with phases for the roots.
>>> +        """
>>> +        cl = repo.changelog
>>> +        phaserevs = self._phaserevs
>>> +        for phase in trackedphases:
>>> +            roots = map(cl.rev, self.phaseroots[phase])
>>> +            for root in roots:
>>> +                phaserevs[root] = phase
>>> +                # We know that the parent of a root has a phase less
>>> than
>>> +                # the root, so if phase == 1, the parent must be
>>> phase 0.
>>> +                if phase == 1:
>>> +                    for parent in cl.parentrevs(root):
>>> +                        if parent != -1:
>>> +                            phaserevs[parent] = 0
>>> +
>>
>> The second part look like an addition and does not belong into this
>> code movement patch. Should be in an independant patches and will fit
>> well in your next series that makes the computation lazy.
>>
>> I've dropped that part and queued the first three.
> I think the phase == 1 logic fits well enough into this patch, without
> increasing it's complexity or reviewability, that requiring it be split
> out is a waste of both of our times.

This patch is about code movement and should remains code movement only 
for clarity purpose. Splitting the  patches into two is trivial and save 
us the necessity to determine if mixing the two is safe enough. Moreover 
there is a significant chance than a "safe enough" change are actually 
not that safe. So better not take risk. Finally, have two patches would 
let your explicitly introduce this small changes instead of sneaking it 
into an unrelated change.

The time I spent figuring out where this extra part was coming from + 
this arguments is already taking more time that producing separated 
patches. Please stick to the "one change per patch" policy of this project.

-- 
Pierre-Yves David