Patchwork [3,of,3] repoview: add optional hiddencache verification

login
register
mail settings
Submitter David Soria Parra
Date Aug. 7, 2014, 6:08 p.m.
Message ID <5dd7cb3ba93256f916a9.1407434927@dev544.prn1.facebook.com>
Download mbox | patch
Permalink /patch/5313/
State Accepted
Headers show

Comments

David Soria Parra - Aug. 7, 2014, 6:08 p.m.
# HG changeset patch
# User David Soria Parra <davidsp@fb.com>
# Date 1407430439 25200
#      Thu Aug 07 09:53:59 2014 -0700
# Node ID 5dd7cb3ba93256f916a970a4dc1f1b7e178e4176
# Parent  3da6820d38112cddfc21e2b99ab02957f93d80cf
repoview: add optional hiddencache verification

To ensure our cache approach is correct and users aren't affected, we are
verifying the cache by default. If we don't encounter any user reports of an
invalid cache we will remove the verification and the internal variable.
Pierre-Yves David - Aug. 8, 2014, 3:47 a.m.
On 08/07/2014 11:08 AM, David Soria Parra wrote:
> # HG changeset patch
> # User David Soria Parra <davidsp@fb.com>
> # Date 1407430439 25200
> #      Thu Aug 07 09:53:59 2014 -0700
> # Node ID 5dd7cb3ba93256f916a970a4dc1f1b7e178e4176
> # Parent  3da6820d38112cddfc21e2b99ab02957f93d80cf
> repoview: add optional hiddencache verification
>
> To ensure our cache approach is correct and users aren't affected, we are
> verifying the cache by default. If we don't encounter any user reports of an
> invalid cache we will remove the verification and the internal variable.
>
> diff --git a/mercurial/repoview.py b/mercurial/repoview.py
> --- a/mercurial/repoview.py
> +++ b/mercurial/repoview.py
> @@ -86,11 +86,19 @@
>           if iscachevalid(repo, hideable):
>               data = repo.vfs.read(cachefile)
>               hidden = frozenset(struct.unpack('%sI' % (len(data) / 4), data))
> +            # internal: don't document as it will be removed in the future.
> +            if repo.ui.configbool('repoview', 'verifycache', True):
> +                blocked = cl.ancestors(_gethiddenblockers(repo), inclusive=True)
> +                computed = frozenset(r for r in hideable if r not in blocked)
> +                if computed != hidden:
> +                    invalidatecache(repo)
> +                    repo.ui.warn(_('Cache inconsistency detected. Please ' +
> +                        'open an issue on http://bz.selenic.com.\n'))
> +                    hidden = computed
>           else:
>               # recompute cache
>               blocked = cl.ancestors(_gethiddenblockers(repo), inclusive=True)
>               hidden = frozenset(r for r in hideable if r not in blocked)
> -
>               # write cache to file
>               try:
>                   data = struct.pack('%sI' % len(hidden), *hidden)

1. We have a "experimental" config section for such purpose. something 
like `experimental.cachehidden` would do.

2. You are not writing the new result in case of invalid cache

3. Given that the current default should be "not use the cache" for now 
I would see the current code structure (some function just exists for 
the speudo code)::

   hidden = hiddencomputation(…)
   if repo.ui.config('experimental', 'cachehidden', False):
       cache = readcache(…) # or return None if missing or invalid
       if cache is not None and cache != hidden:
            repo.ui.warn(…)
            cache = None
       if cache is None:
            writecache(…)
   return hidden

such structure will be easy to translate when we drop the config option:

   hidden = readcache(…) # or return None if missing or invalid
   if hidden is None:
        hidden = hiddencomputation(…)
        writecache(…)
   return hidden

This implies merging patches 2 and 3. (the cache logic -may- be 
partially introduced as uncalled function in prior patches if you want 
to focus on series readability)

Patch

diff --git a/mercurial/repoview.py b/mercurial/repoview.py
--- a/mercurial/repoview.py
+++ b/mercurial/repoview.py
@@ -86,11 +86,19 @@ 
         if iscachevalid(repo, hideable):
             data = repo.vfs.read(cachefile)
             hidden = frozenset(struct.unpack('%sI' % (len(data) / 4), data))
+            # internal: don't document as it will be removed in the future.
+            if repo.ui.configbool('repoview', 'verifycache', True):
+                blocked = cl.ancestors(_gethiddenblockers(repo), inclusive=True)
+                computed = frozenset(r for r in hideable if r not in blocked)
+                if computed != hidden:
+                    invalidatecache(repo)
+                    repo.ui.warn(_('Cache inconsistency detected. Please ' +
+                        'open an issue on http://bz.selenic.com.\n'))
+                    hidden = computed
         else:
             # recompute cache
             blocked = cl.ancestors(_gethiddenblockers(repo), inclusive=True)
             hidden = frozenset(r for r in hideable if r not in blocked)
-
             # write cache to file
             try:
                 data = struct.pack('%sI' % len(hidden), *hidden)