Patchwork [10,of,10] branchmap: move validity logic in the object itself

# HG changeset patch
# User Pierre-Yves David <pierre-yves.david at ens-lyon.org>
# Date 1356140640 -3600
# Node ID 08bed0e9a2d5aa130dc84bac6ec5da8e3be7b832
# Parent  5405ded918f63d80b075cd85edf635bbd87eec7b
branchmap: move validity logic in the object itself

In several place, We check if a branchcache is still valid regarding the current
state of the repository. This changeset puts this logic in a method of the object
that can be reused when necessary.

A branch map is considered valid whenever it is up to date or a strict subset of
the repository state.

The change will help making branchcache aware of filtered revision.

Other than my question on patch 4 and Dave S's English tweaks, I think this series is probably a good start, but I wonder why read isn't moved into the branchmap class as well? Or maybe I overlooked that.

On Dec 21, 2012, at 8:48 PM, Pierre-Yves David <pierre-yves.david at ens-lyon.org> wrote:

> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david at ens-lyon.org>
> # Date 1356140640 -3600
> # Node ID 08bed0e9a2d5aa130dc84bac6ec5da8e3be7b832
> # Parent  5405ded918f63d80b075cd85edf635bbd87eec7b
> branchmap: move validity logic in the object itself
> 
> In several place, We check if a branchcache is still valid regarding the current
> state of the repository. This changeset puts this logic in a method of the object
> that can be reused when necessary.
> 
> A branch map is considered valid whenever it is up to date or a strict subset of
> the repository state.
> 
> The change will help making branchcache aware of filtered revision.
> 
> diff --git a/mercurial/branchmap.py b/mercurial/branchmap.py
> --- a/mercurial/branchmap.py
> +++ b/mercurial/branchmap.py
> @@ -7,22 +7,22 @@
> 
> from node import bin, hex, nullid, nullrev
> import encoding
> 
> def read(repo):
> -    partial = branchcache()
>     try:
>         f = repo.opener("cache/branchheads")
>         lines = f.read().split('\n')
>         f.close()
>     except (IOError, OSError):
>         return branchcache()
> 
>     try:
>         last, lrev = lines.pop(0).split(" ", 1)
>         last, lrev = bin(last), int(lrev)
> -        if lrev >= len(repo) or repo[lrev].node() != last:
> +        partial = branchcache(tipnode=last, tiprev=lrev)
> +        if not partial.validfor(repo):
>             # invalidate the cache
>             raise ValueError('invalidating branch cache (tip differs)')
>         for l in lines:
>             if not l:
>                 continue
> @@ -30,12 +30,10 @@ def read(repo):
>             label = encoding.tolocal(label.strip())
>             if not node in repo:
>                 raise ValueError('invalidating branch cache because node '+
>                                  '%s does not exist' % node)
>             partial.setdefault(label, []).append(bin(node))
> -        partial.tipnode = last
> -        partial.tiprev = lrev
>     except KeyboardInterrupt:
>         raise
>     except Exception, inst:
>         if repo.ui.debugflag:
>             repo.ui.warn(str(inst), '\n')
> @@ -45,16 +43,13 @@ def read(repo):
> 
> 
> def updatecache(repo):
>     repo = repo.unfiltered()  # Until we get a smarter cache management
>     cl = repo.changelog
> -    tip = cl.tip()
>     partial = repo._branchcache
> -    if partial is not None and partial.tipnode == tip:
> -        return
> 
> -    if partial is None or partial.tipnode not in cl.nodemap:
> +    if partial is None or partial.validfor(repo):
>         partial = read(repo)
> 
>     catip = repo._cacheabletip()
>     # if partial.tiprev == catip: cache is already up to date
>     # if partial.tiprev >  catip: we have uncachable element in `partial` can't
> @@ -78,10 +73,21 @@ class branchcache(dict):
>     def __init__(self, entries=(), tipnode=nullid, tiprev=nullrev):
>         super(branchcache, self).__init__(entries)
>         self.tipnode = tipnode
>         self.tiprev = tiprev
> 
> +    def validfor(self, repo):
> +        """Is the cache content valide regarding a repo
> +
> +        - False when cached tipnode are unknown or if we detect a strip.
> +        - True when cache is up to date or a subset of current repo."""
> +        try:
> +            return self.tipnode == repo.changelog.node(self.tiprev)
> +        except IndexError:
> +            return False
> +
> +
>     def write(self, repo):
>         try:
>             f = repo.opener("cache/branchheads", "w", atomictemp=True)
>             f.write("%s %s\n" % (hex(self.tipnode), self.tiprev))
>             for label, nodes in self.iteritems():
> @@ -160,12 +166,12 @@ class branchcache(dict):
>                 del self[branch]
>         try:
>             node = cl.node(self.tiprev)
>         except IndexError:
>             node = None
> -        if ((self.tipnode != node)
> -            or (self.tipnode in droppednodes)):
> +        if ((not self.validfor(repo)) or (self.tipnode in droppednodes)):
> +
>             # cache key are not valid anymore
>             self.tipnode = nullid
>             self.tiprev = nullrev
>             for heads in self.values():
>                 tiprev = max(cl.rev(node) for node in heads)

On 24 d?c. 2012, at 02:26, Augie Fackler wrote:

> Other than my question on patch 4 and Dave S's English tweaks, I think this series is probably a good start.

I'll sent a V2 with english change and fix for patch 10 shortly.

>  but I wonder why read isn't moved into the branchmap class as well? Or maybe I overlooked that.

Read is function that take a repo to create a branchcache object. In later series it may even returns None when the on disk cache is invalid.

We could imagine that `read` could be a constructor that raise an exception when the cache is invalid.

However, I prefer to focus on change necessary for filtering. When change necessary for filtering will be done, we'll wait for the dust to settles and finish cleaning up the module.

Doing this before is (1) a bit prematurate regarding coming changes (2) not on the critical path of filtering.