Patchwork [09,of,18,V2] phases: make _filterunknown a member function of phasecache

login
register
mail settings
Submitter Pierre-Yves David
Date Jan. 4, 2013, 1:04 a.m.
Message ID <733b35c15776bb62518c.1357261452@yamac.lan>
Download mbox | patch
Permalink /patch/378/
State Accepted
Commit 767d1c602c8bcee7abbf36a44c4c06a5127f5a83
Headers show

Comments

Pierre-Yves David - Jan. 4, 2013, 1:04 a.m.
# HG changeset patch
# User Idan Kamara <idankk86 at gmail.com>
# Date 1355682488 -7200
# Node ID 733b35c15776bb62518cf3e544022d41e717b7d7
# Parent  f33473072cb625b64b9ded2d4eb054f42c735a6c
phases: make _filterunknown a member function of phasecache

We'd like the ability to call filterunknown on an existing phasecache
instance after nodes are destroyed.
Kevin Bullock - Jan. 4, 2013, 4:16 a.m.
On 3 Jan 2013, at 7:04 PM, Pierre-Yves David wrote:

> # HG changeset patch
> # User Idan Kamara <idankk86 at gmail.com>
> # Date 1355682488 -7200
> # Node ID 733b35c15776bb62518cf3e544022d41e717b7d7
> # Parent  f33473072cb625b64b9ded2d4eb054f42c735a6c
> phases: make _filterunknown a member function of phasecache
> 
> We'd like the ability to call filterunknown on an existing phasecache
> instance after nodes are destroyed.
> 
> diff --git a/mercurial/phases.py b/mercurial/phases.py
> --- a/mercurial/phases.py
> +++ b/mercurial/phases.py
> [...]
> @@ -154,19 +136,18 @@ def _readroots(repo, phasedefaults=None)
>             raise
>         if phasedefaults:
>             for f in phasedefaults:
>                 roots = f(repo, roots)
>         dirty = True
> -    if _filterunknown(repo.ui, repo.changelog, roots):
> -        dirty = True
>     return roots, dirty
> 
> class phasecache(object):
>     def __init__(self, repo, phasedefaults, _load=True):
>         if _load:
>             # Cheap trick to allow shallow-copy without copy module
>             self.phaseroots, self.dirty = _readroots(repo, phasedefaults)
> +            self.filterunknown(repo)
>             self.opener = repo.sopener
>             self._phaserevs = None
Is this line still necessary after we've folded it into filterunknown below?

pacem in terris / ??? / ?????? / ????????? / ??
Kevin R. Bullock
Pierre-Yves David - Jan. 4, 2013, 4:21 a.m.
On 4 janv. 2013, at 05:16, Kevin Bullock wrote:

> On 3 Jan 2013, at 7:04 PM, Pierre-Yves David wrote:
> 
>> # HG changeset patch
>> # User Idan Kamara <idankk86 at gmail.com>
>> # Date 1355682488 -7200
>> # Node ID 733b35c15776bb62518cf3e544022d41e717b7d7
>> # Parent  f33473072cb625b64b9ded2d4eb054f42c735a6c
>> phases: make _filterunknown a member function of phasecache
>> 
>> We'd like the ability to call filterunknown on an existing phasecache
>> instance after nodes are destroyed.
>> 
>> diff --git a/mercurial/phases.py b/mercurial/phases.py
>> --- a/mercurial/phases.py
>> +++ b/mercurial/phases.py
>> [...]
>> @@ -154,19 +136,18 @@ def _readroots(repo, phasedefaults=None)
>>            raise
>>        if phasedefaults:
>>            for f in phasedefaults:
>>                roots = f(repo, roots)
>>        dirty = True
>> -    if _filterunknown(repo.ui, repo.changelog, roots):
>> -        dirty = True
>>    return roots, dirty
>> 
>> class phasecache(object):
>>    def __init__(self, repo, phasedefaults, _load=True):
>>        if _load:
>>            # Cheap trick to allow shallow-copy without copy module
>>            self.phaseroots, self.dirty = _readroots(repo, phasedefaults)
>> +            self.filterunknown(repo)
>>            self.opener = repo.sopener
>>            self._phaserevs = None
> Is this line still necessary after we've folded it into filterunknown below?

Very much.
(1) The function is moved on the object.
(2) the function call is moved from _readroots to the _readroots caller: __init__
Kevin Bullock - Jan. 4, 2013, 4:40 a.m.
On 3 Jan 2013, at 10:21 PM, Pierre-Yves David wrote:

> On 4 janv. 2013, at 05:16, Kevin Bullock wrote:
> 
>> On 3 Jan 2013, at 7:04 PM, Pierre-Yves David wrote:
>> 
>>> # HG changeset patch
>>> # User Idan Kamara <idankk86 at gmail.com>
>>> # Date 1355682488 -7200
>>> # Node ID 733b35c15776bb62518cf3e544022d41e717b7d7
>>> # Parent  f33473072cb625b64b9ded2d4eb054f42c735a6c
>>> phases: make _filterunknown a member function of phasecache
>>> 
>>> We'd like the ability to call filterunknown on an existing phasecache
>>> instance after nodes are destroyed.
>>> 
>>> diff --git a/mercurial/phases.py b/mercurial/phases.py
>>> --- a/mercurial/phases.py
>>> +++ b/mercurial/phases.py
>>> [...]
>>> @@ -154,19 +136,18 @@ def _readroots(repo, phasedefaults=None)
>>>           raise
>>>       if phasedefaults:
>>>           for f in phasedefaults:
>>>               roots = f(repo, roots)
>>>       dirty = True
>>> -    if _filterunknown(repo.ui, repo.changelog, roots):
>>> -        dirty = True
>>>   return roots, dirty
>>> 
>>> class phasecache(object):
>>>   def __init__(self, repo, phasedefaults, _load=True):
>>>       if _load:
>>>           # Cheap trick to allow shallow-copy without copy module
>>>           self.phaseroots, self.dirty = _readroots(repo, phasedefaults)
>>> +            self.filterunknown(repo)
>>>           self.opener = repo.sopener
>>>           self._phaserevs = None
>> Is this line still necessary after we've folded it into filterunknown below?
> 
> Very much.
> (1) The function is moved on the object.
> (2) the function call is moved from _readroots to the _readroots caller: __init__

I meant the repeated 'self._phaserevs = None' line.

pacem in terris / ??? / ?????? / ????????? / ??
Kevin R. Bullock
Pierre-Yves David - Jan. 4, 2013, 4:57 a.m.
On 4 janv. 2013, at 05:40, Kevin Bullock wrote:

> On 3 Jan 2013, at 10:21 PM, Pierre-Yves David wrote:
> 
>> On 4 janv. 2013, at 05:16, Kevin Bullock wrote:
>> 
>>> On 3 Jan 2013, at 7:04 PM, Pierre-Yves David wrote:
>>> 
>>>> # HG changeset patch
>>>> # User Idan Kamara <idankk86 at gmail.com>
>>>> # Date 1355682488 -7200
>>>> # Node ID 733b35c15776bb62518cf3e544022d41e717b7d7
>>>> # Parent  f33473072cb625b64b9ded2d4eb054f42c735a6c
>>>> phases: make _filterunknown a member function of phasecache
>>>> 
>>>> We'd like the ability to call filterunknown on an existing phasecache
>>>> instance after nodes are destroyed.
>>>> 
>>>> diff --git a/mercurial/phases.py b/mercurial/phases.py
>>>> --- a/mercurial/phases.py
>>>> +++ b/mercurial/phases.py
>>>> [...]
>>>> @@ -154,19 +136,18 @@ def _readroots(repo, phasedefaults=None)
>>>>          raise
>>>>      if phasedefaults:
>>>>          for f in phasedefaults:
>>>>              roots = f(repo, roots)
>>>>      dirty = True
>>>> -    if _filterunknown(repo.ui, repo.changelog, roots):
>>>> -        dirty = True
>>>>  return roots, dirty
>>>> 
>>>> class phasecache(object):
>>>>  def __init__(self, repo, phasedefaults, _load=True):
>>>>      if _load:
>>>>          # Cheap trick to allow shallow-copy without copy module
>>>>          self.phaseroots, self.dirty = _readroots(repo, phasedefaults)
>>>> +            self.filterunknown(repo)
>>>>          self.opener = repo.sopener
>>>>          self._phaserevs = None
>>> Is this line still necessary after we've folded it into filterunknown below?
>> 
>> Very much.
>> (1) The function is moved on the object.
>> (2) the function call is moved from _readroots to the _readroots caller: __init__
> 
> I meant the repeated 'self._phaserevs = None' line.

I would move it before the filterunknown call. I hate object that use attribute not declared in the __init__

Patch

diff --git a/mercurial/phases.py b/mercurial/phases.py
--- a/mercurial/phases.py
+++ b/mercurial/phases.py
@@ -107,28 +107,10 @@  import util, error
 
 allphases = public, draft, secret = range(3)
 trackedphases = allphases[1:]
 phasenames = ['public', 'draft', 'secret']
 
-def _filterunknown(ui, changelog, phaseroots):
-    """remove unknown nodes from the phase boundary
-
-    Nothing is lost as unknown nodes only hold data for their descendants.
-    """
-    updated = False
-    nodemap = changelog.nodemap # to filter unknown nodes
-    for phase, nodes in enumerate(phaseroots):
-        missing = [node for node in nodes if node not in nodemap]
-        if missing:
-            for mnode in missing:
-                ui.debug(
-                    'removing unknown node %s from %i-phase boundary\n'
-                    % (short(mnode), phase))
-            nodes.symmetric_difference_update(missing)
-            updated = True
-    return updated
-
 def _readroots(repo, phasedefaults=None):
     """Read phase roots from disk
 
     phasedefaults is a list of fn(repo, roots) callable, which are
     executed if the phase roots file does not exist. When phases are
@@ -154,19 +136,18 @@  def _readroots(repo, phasedefaults=None)
             raise
         if phasedefaults:
             for f in phasedefaults:
                 roots = f(repo, roots)
         dirty = True
-    if _filterunknown(repo.ui, repo.changelog, roots):
-        dirty = True
     return roots, dirty
 
 class phasecache(object):
     def __init__(self, repo, phasedefaults, _load=True):
         if _load:
             # Cheap trick to allow shallow-copy without copy module
             self.phaseroots, self.dirty = _readroots(repo, phasedefaults)
+            self.filterunknown(repo)
             self.opener = repo.sopener
             self._phaserevs = None
 
     def copy(self):
         # Shallow copy meant to ensure isolation in
@@ -265,10 +246,30 @@  class phasecache(object):
             ctxs = repo.set('roots(%ln::)', currentroots)
             currentroots.intersection_update(ctx.node() for ctx in ctxs)
             self._updateroots(targetphase, currentroots)
         repo.invalidatevolatilesets()
 
+    def filterunknown(self, repo):
+        """remove unknown nodes from the phase boundary
+
+        Nothing is lost as unknown nodes only hold data for their descendants.
+        """
+        filtered = False
+        nodemap = repo.changelog.nodemap # to filter unknown nodes
+        for phase, nodes in enumerate(self.phaseroots):
+            missing = [node for node in nodes if node not in nodemap]
+            if missing:
+                for mnode in missing:
+                    repo.ui.debug(
+                        'removing unknown node %s from %i-phase boundary\n'
+                        % (short(mnode), phase))
+                nodes.symmetric_difference_update(missing)
+                filtered = True
+        if filtered:
+            self.dirty = True
+            self._phaserevs = None
+
 def advanceboundary(repo, targetphase, nodes):
     """Add nodes to a phase changing other nodes phases if necessary.
 
     This function move boundary *forward* this means that all nodes
     are set in the target phase or kept in a *lower* phase.