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

login
register
mail settings
Submitter Durham Goode
Date Oct. 9, 2014, 7:22 p.m.
Message ID <9291d6a89c9b9bf93cbb.1412882561@dev2000.prn2.facebook.com>
Download mbox | patch
Permalink /patch/6170/
State Accepted
Headers show

Comments

Durham Goode - Oct. 9, 2014, 7:22 p.m.
# HG changeset patch
# User Durham Goode <durham@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.
Pierre-Yves David - Oct. 13, 2014, 6:33 a.m.
On 10/09/2014 12:22 PM, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@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.

>       def phase(self, repo, rev):
>           # We need a repo argument here to be able to build _phaserevs
>           # if necessary. The repository instance is not stored in
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>
Durham Goode - Oct. 13, 2014, 5:59 p.m.
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@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.
Pierre-Yves David - Oct. 13, 2014, 6:12 p.m.
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@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.

Patch

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
+
     def phase(self, repo, rev):
         # We need a repo argument here to be able to build _phaserevs
         # if necessary. The repository instance is not stored in