[PATCH 3 of 3] repoview: add optional hiddencache verification

[Pierre-Yves David]

On 08/07/2014 11:08 AM, David Soria Parra wrote:
> # HG changeset patch
> # User David Soria Parra <davidsp at 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)

-- 
Pierre-Yves David