Patchwork [05,of,12] phases: make _filterunknown a member function of phasecache

login
register
mail settings
Submitter Idan Kamara
Date Dec. 17, 2012, 3:35 p.m.
Message ID <ded30a7d2cc2d32e20bf.1355758530@idan>
Download mbox | patch
Permalink /patch/159/
State Superseded
Commit 767d1c602c8bcee7abbf36a44c4c06a5127f5a83
Headers show

Comments

Idan Kamara - Dec. 17, 2012, 3:35 p.m.
# HG changeset patch
# User Idan Kamara <idankk86 at gmail.com>
# Date 1355682488 -7200
# Branch stable
# Node ID ded30a7d2cc2d32e20bf6a14b9579e10210a0673
# Parent  aa6a85e47fb4edc705959da93fb024aef56dafd4
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 - Dec. 17, 2012, 5:12 p.m.
On Dec 17, 2012, at 9:35 AM, Idan Kamara wrote:

> # HG changeset patch
> # User Idan Kamara <idankk86 at gmail.com>
> # Date 1355682488 -7200
> # Branch stable
> # Node ID ded30a7d2cc2d32e20bf6a14b9579e10210a0673
> # Parent  aa6a85e47fb4edc705959da93fb024aef56dafd4
> 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
> @@ -110,24 +110,6 @@
> 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
> 
> @@ -156,8 +138,6 @@
>             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):
> @@ -165,6 +145,7 @@
>         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
> 
> @@ -264,6 +245,26 @@
>             self._updateroots(targetphase, currentroots)
>         obsolete.clearobscaches(repo)
> 
> +    def filterunknown(self, repo):

This adds yet another method to the phasecache that takes a 'repo' argument. Would it be appropriate to make the constructor keep a handle on the repo?

pacem in terris / ??? / ?????? / ????????? / ??
Kevin R. Bullock
Pierre-Yves David - Dec. 17, 2012, 5:16 p.m.
On Mon, Dec 17, 2012 at 11:12:46AM -0600, Kevin Bullock wrote:
> On Dec 17, 2012, at 9:35 AM, Idan Kamara wrote:
> > +    def filterunknown(self, repo):
> 
> This adds yet another method to the phasecache that takes a 'repo' argument. Would it be appropriate to make the constructor keep a handle on the repo?

That would need to be a weakref. And Matt does not like weakref[1].
Matt Mackall - Dec. 18, 2012, 10:32 p.m.
On Mon, 2012-12-17 at 18:16 +0100, Pierre-Yves David wrote:
> On Mon, Dec 17, 2012 at 11:12:46AM -0600, Kevin Bullock wrote:
> > On Dec 17, 2012, at 9:35 AM, Idan Kamara wrote:
> > > +    def filterunknown(self, repo):
> > 
> > This adds yet another method to the phasecache that takes a 'repo' argument. Would it be appropriate to make the constructor keep a handle on the repo?
> 
> That would need to be a weakref. And Matt does not like weakref[1].

I certainly consider them to be a tool of last resort.

Patch

diff --git a/mercurial/phases.py b/mercurial/phases.py
--- a/mercurial/phases.py
+++ b/mercurial/phases.py
@@ -110,24 +110,6 @@ 
 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
 
@@ -156,8 +138,6 @@ 
             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):
@@ -165,6 +145,7 @@ 
         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
 
@@ -264,6 +245,26 @@ 
             self._updateroots(targetphase, currentroots)
         obsolete.clearobscaches(repo)
 
+    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.