Patchwork [01,of,10] branchmap: simplify _branchtags using a new _cachabletip method

login
register
mail settings
Submitter Pierre-Yves David
Date Dec. 19, 2012, 1:53 p.m.
Message ID <56c016dea0622854146f.1355925197@crater1.logilab.fr>
Download mbox | patch
Permalink /patch/192/
State Superseded, archived
Headers show

Comments

Pierre-Yves David - Dec. 19, 2012, 1:53 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david at ens-lyon.org>
# Date 1355923841 -3600
# Node ID 56c016dea0622854146f52acb9b3d5c9258636b9
# Parent  9c76da468a19a0cf9551b8a667d2913b0b0ee7be
branchmap: simplify _branchtags using a new _cachabletip method

The current _branchtags method is a remain of an older much larger function. Its
only remaining role is to help MQ to alter the version of branchcache we store
on disk. As MQ mutate the repository it ensures persistent cache does not
contains anything it is likely to alter.

This changeset explicits the stable vs volatile part of repository and reduces the
MQ specific code to the computation of this limit. The main _branchtags code now
handles this possible limits in all case.

This will helps to extract the branchmap logic from the repository.

The new code of _branchtags is a bit duplicated, but as I expect major
refactoring of this section I'm not keen to setup factorisation function here.
Kevin Bullock - Dec. 20, 2012, 4:24 a.m.
On Dec 19, 2012, at 7:53 AM, pierre-yves.david at logilab.fr wrote:

> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david at ens-lyon.org>
> # Date 1355923841 -3600
> # Node ID 56c016dea0622854146f52acb9b3d5c9258636b9
> # Parent  9c76da468a19a0cf9551b8a667d2913b0b0ee7be
> branchmap: simplify _branchtags using a new _cachabletip method

[Compulsive copy-editing follows. Sorry, I can't help it.]
Should be _cacheabletip (note the 'e'). [Google returns 2.5e6 results with the 'e', only 430k without it. Looks weird to me without it. Oh, also, we use 'cacheable' in scmutil.]

Actually, now reading what the method does, maybe something more like '_lastcacheable' ?

> The current _branchtags method is a remain of an older much larger function. Its
                                      remnant      older, much
[Bonus points for using the correct 'its' though. Most native speakers get that consistently wrong.]

> only remaining role is to help MQ to alter the version of branchcache we store
> on disk. As MQ mutate the repository it ensures persistent cache does not
                 mutates
> contains anything it is likely to alter.
  contain

> This changeset explicits the stable vs volatile part of repository and reduces the
                 makes explicit[?]
> MQ specific code to the computation of this limit. The main _branchtags code now
> handles this possible limits in all case.
                        limit         cases.

> This will helps to extract the branchmap logic from the repository.
            help to

> The new code of _branchtags is a bit duplicated, but as I expect major
> refactoring of this section I'm not keen to setup factorisation function here.
> 
> diff --git a/hgext/mq.py b/hgext/mq.py
> --- a/hgext/mq.py
> +++ b/hgext/mq.py
> @@ -3475,11 +3475,11 @@ def reposetup(ui, repo):
>                 else:
>                     tags[patch[1]] = patch[0]
> 
>             return result
> 
> -        def _branchtags(self, partial, lrev):
> +        def _cachabletip(self):
>             q = self.mq
>             cl = self.changelog
>             qbase = None
>             if not q.applied:
>                 if getattr(self, '_committingpatch', False):
> @@ -3490,29 +3490,14 @@ def reposetup(ui, repo):
>                 try:
>                     qbase = self.unfiltered().changelog.rev(qbasenode)
>                 except error.LookupError:
>                     self.ui.warn(_('mq status file refers to unknown node %s\n')
>                                  % short(qbasenode))
> -            if qbase is None:
> -                return super(mqrepo, self)._branchtags(partial, lrev)
> -
> -            start = lrev + 1
> -            if start < qbase:
> -                # update the cache (excluding the patches) and save it
> -                ctxgen = (self[r] for r in xrange(lrev + 1, qbase))
> -                self._updatebranchcache(partial, ctxgen)
> -                self._writebranchcache(partial, cl.node(qbase - 1), qbase - 1)
> -                start = qbase
> -            # if start = qbase, the cache is as updated as it should be.
> -            # if start > qbase, the cache includes (part of) the patches.
> -            # we might as well use it, but we won't save it.
> -
> -            # update the cache up to the tip
> -            ctxgen = (self[r] for r in xrange(start, len(cl)))
> -            self._updatebranchcache(partial, ctxgen)
> -
> -            return partial
> +            ret = super(mqrepo, self)._cachabletip()
> +            if qbase is not None:
> +                ret = min(qbase - 1, ret)
> +            return ret
> 
>     if repo.local():
>         repo.__class__ = mqrepo
> 
>         repo._phasedefaults.append(mqphasedefaults)
> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -630,18 +630,38 @@ class localrepository(object):
>         for bookmark, n in self._bookmarks.iteritems():
>             if n == node:
>                 marks.append(bookmark)
>         return sorted(marks)
> 
> +    def _cachabletip(self):
> +        """tip-most revision stable enought to used in persistent cache
> +
> +        This function is overwritten by MQ to ensure we do not write cache for
> +        a part of the history that will likely change.
> +
> +        Efficient handling of filtered revision in branchcache should offer a
> +        better alternative. But we are using this approach until it is ready.
> +        """
> +        cl = self.changelog
> +        return cl.rev(cl.tip())
> +
>     def _branchtags(self, partial, lrev):
>         # TODO: rename this function?
> +        cl = self.changelog
> +        catip = self._cachabletip()
> +        # if lrev == catip: cache is already up to date
> +        # if lrev >  catip: we have uncachable element in `partial` can't write
> +        #                   on disk
> +        if lrev < catip:
> +            ctxgen = (self[r] for r in cl.revs(lrev + 1, catip))
> +            self._updatebranchcache(partial, ctxgen)
> +            self._writebranchcache(partial, cl.node(catip), catip)
> +        # update cache up to current tip
>         tiprev = len(self) - 1
> -        if lrev != tiprev:
> -            ctxgen = (self[r] for r in self.changelog.revs(lrev + 1, tiprev))
> +        if lrev < tiprev:
> +            ctxgen = (self[r] for r in cl.revs(lrev + 1, tiprev))
>             self._updatebranchcache(partial, ctxgen)
Why are we doing this again? I'm not suggesting it be factored out, just wondering why we're updating the branchcache for tip, _without_ writing it to disk here.

> -            self._writebranchcache(partial, self.changelog.tip(), tiprev)
> -
>         return partial
> 
>     @unfilteredmethod # Until we get a smarter cache management
>     def updatebranchcache(self):
>         tip = self.changelog.tip()
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel

pacem in terris / ??? / ?????? / ????????? / ??
Kevin R. Bullock
Pierre-Yves David - Dec. 20, 2012, 11:16 a.m.
On Wed, Dec 19, 2012 at 10:24:24PM -0600, Kevin Bullock wrote:
> On Dec 19, 2012, at 7:53 AM, pierre-yves.david at logilab.fr wrote:
> 
> > # HG changeset patch
> > # User Pierre-Yves David <pierre-yves.david at ens-lyon.org>
> > # Date 1355923841 -3600
> > # Node ID 56c016dea0622854146f52acb9b3d5c9258636b9
> > # Parent  9c76da468a19a0cf9551b8a667d2913b0b0ee7be
> > branchmap: simplify _branchtags using a new _cachabletip method
> 
> [Compulsive copy-editing follows. Sorry, I can't help it.]
> Should be _cacheabletip (note the 'e'). [Google returns 2.5e6 results with the 'e', only 430k without it. Looks weird to me without it. Oh, also, we use 'cacheable' in scmutil.]

we usually revert to "lasted" revision as tip or tip-most. I think _cacheabletip is fine here.

I fixed other feedback thanks.

> > -        if lrev != tiprev:
> > -            ctxgen = (self[r] for r in self.changelog.revs(lrev + 1, tiprev))
> > +        if lrev < tiprev:
> > +            ctxgen = (self[r] for r in cl.revs(lrev + 1, tiprev))
> >             self._updatebranchcache(partial, ctxgen)
> Why are we doing this again? I'm not suggesting it be factored out, just wondering why we're updating the branchcache for tip, _without_ writing it to disk here.

Hum lrev should have been update to the value of catip after the first update.
I fixed that and added a comment in the newer version to make it more
explicite.

Patch

diff --git a/hgext/mq.py b/hgext/mq.py
--- a/hgext/mq.py
+++ b/hgext/mq.py
@@ -3475,11 +3475,11 @@  def reposetup(ui, repo):
                 else:
                     tags[patch[1]] = patch[0]
 
             return result
 
-        def _branchtags(self, partial, lrev):
+        def _cachabletip(self):
             q = self.mq
             cl = self.changelog
             qbase = None
             if not q.applied:
                 if getattr(self, '_committingpatch', False):
@@ -3490,29 +3490,14 @@  def reposetup(ui, repo):
                 try:
                     qbase = self.unfiltered().changelog.rev(qbasenode)
                 except error.LookupError:
                     self.ui.warn(_('mq status file refers to unknown node %s\n')
                                  % short(qbasenode))
-            if qbase is None:
-                return super(mqrepo, self)._branchtags(partial, lrev)
-
-            start = lrev + 1
-            if start < qbase:
-                # update the cache (excluding the patches) and save it
-                ctxgen = (self[r] for r in xrange(lrev + 1, qbase))
-                self._updatebranchcache(partial, ctxgen)
-                self._writebranchcache(partial, cl.node(qbase - 1), qbase - 1)
-                start = qbase
-            # if start = qbase, the cache is as updated as it should be.
-            # if start > qbase, the cache includes (part of) the patches.
-            # we might as well use it, but we won't save it.
-
-            # update the cache up to the tip
-            ctxgen = (self[r] for r in xrange(start, len(cl)))
-            self._updatebranchcache(partial, ctxgen)
-
-            return partial
+            ret = super(mqrepo, self)._cachabletip()
+            if qbase is not None:
+                ret = min(qbase - 1, ret)
+            return ret
 
     if repo.local():
         repo.__class__ = mqrepo
 
         repo._phasedefaults.append(mqphasedefaults)
diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -630,18 +630,38 @@  class localrepository(object):
         for bookmark, n in self._bookmarks.iteritems():
             if n == node:
                 marks.append(bookmark)
         return sorted(marks)
 
+    def _cachabletip(self):
+        """tip-most revision stable enought to used in persistent cache
+
+        This function is overwritten by MQ to ensure we do not write cache for
+        a part of the history that will likely change.
+
+        Efficient handling of filtered revision in branchcache should offer a
+        better alternative. But we are using this approach until it is ready.
+        """
+        cl = self.changelog
+        return cl.rev(cl.tip())
+
     def _branchtags(self, partial, lrev):
         # TODO: rename this function?
+        cl = self.changelog
+        catip = self._cachabletip()
+        # if lrev == catip: cache is already up to date
+        # if lrev >  catip: we have uncachable element in `partial` can't write
+        #                   on disk
+        if lrev < catip:
+            ctxgen = (self[r] for r in cl.revs(lrev + 1, catip))
+            self._updatebranchcache(partial, ctxgen)
+            self._writebranchcache(partial, cl.node(catip), catip)
+        # update cache up to current tip
         tiprev = len(self) - 1
-        if lrev != tiprev:
-            ctxgen = (self[r] for r in self.changelog.revs(lrev + 1, tiprev))
+        if lrev < tiprev:
+            ctxgen = (self[r] for r in cl.revs(lrev + 1, tiprev))
             self._updatebranchcache(partial, ctxgen)
-            self._writebranchcache(partial, self.changelog.tip(), tiprev)
-
         return partial
 
     @unfilteredmethod # Until we get a smarter cache management
     def updatebranchcache(self):
         tip = self.changelog.tip()