Patchwork [10,of,14] obsstore: pass a repository object for initialisation

login
register
mail settings
Submitter Boris Feld
Date July 9, 2017, 5:55 p.m.
Message ID <985d753d4f5799f2a332.1499622922@FB>
Download mbox | patch
Permalink /patch/22171/
State Changes Requested
Headers show

Comments

Boris Feld - July 9, 2017, 5:55 p.m.
# HG changeset patch
# User Boris Feld <boris.feld@octobus.net>
# Date 1495197862 -7200
#      Fri May 19 14:44:22 2017 +0200
# Node ID 985d753d4f5799f2a332140adedb06efd465d62b
# Parent  63214f4d9a766761259b650539eede424413e6a2
# EXP-Topic obs-cache
obsstore: pass a repository object for initialisation

The cache will needs a repository object (to grab a 'vfs'), so we pass a repo object instead of
just the 'svfs' and we grab the 'svfs' from there.
Augie Fackler - July 14, 2017, 6:11 p.m.
On Sun, Jul 09, 2017 at 07:55:22PM +0200, Boris Feld wrote:
> # HG changeset patch
> # User Boris Feld <boris.feld@octobus.net>
> # Date 1495197862 -7200
> #      Fri May 19 14:44:22 2017 +0200
> # Node ID 985d753d4f5799f2a332140adedb06efd465d62b
> # Parent  63214f4d9a766761259b650539eede424413e6a2
> # EXP-Topic obs-cache
> obsstore: pass a repository object for initialisation
>
> The cache will needs a repository object (to grab a 'vfs'), so we
> pass a repo object instead of just the 'svfs' and we grab the 'svfs'
> from there.

I suspect I'll get to it, but why does this cache want to know about
anything outside of svfs?

I'm pretty uncomfortable (architecturally) with passing all of `self`
into the cache layer.

>
> diff -r 63214f4d9a76 -r 985d753d4f57 contrib/perf.py
> --- a/contrib/perf.py	Fri May 19 14:46:26 2017 +0200
> +++ b/contrib/perf.py	Fri May 19 14:44:22 2017 +0200
> @@ -1391,8 +1391,7 @@
>
>      Result is the number of markers in the repo."""
>      timer, fm = gettimer(ui)
> -    svfs = getsvfs(repo)
> -    timer(lambda: len(obsolete.obsstore(svfs)))
> +    timer(lambda: len(obsolete.obsstore(repo)))
>      fm.end()
>
>  @command('perflrucachedict', formatteropts +
> diff -r 63214f4d9a76 -r 985d753d4f57 mercurial/obsolete.py
> --- a/mercurial/obsolete.py	Fri May 19 14:46:26 2017 +0200
> +++ b/mercurial/obsolete.py	Fri May 19 14:44:22 2017 +0200
> @@ -519,10 +519,10 @@
>      # 1024 byte should be about 10 markers in average
>      _obskeyspan = 1024
>
> -    def __init__(self, svfs, defaultformat=_fm1version, readonly=False):
> +    def __init__(self, repo, defaultformat=_fm1version, readonly=False):
>          # caches for various obsolescence related cache
>          self.caches = {}
> -        self.svfs = svfs
> +        self.svfs = repo.svfs
>          self._defaultformat = defaultformat
>          self._readonly = readonly
>
> @@ -799,7 +799,7 @@
>      if defaultformat is not None:
>          kwargs['defaultformat'] = defaultformat
>      readonly = not isenabled(repo, createmarkersopt)
> -    store = obsstore(repo.svfs, readonly=readonly, **kwargs)
> +    store = obsstore(repo, readonly=readonly, **kwargs)
>      if store and readonly:
>          ui.warn(_('obsolete feature not enabled but %i markers found!\n')
>                  % len(list(store)))
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Boris Feld - July 14, 2017, 6:15 p.m.
On Fri, 2017-07-14 at 14:11 -0400, Augie Fackler wrote:
> On Sun, Jul 09, 2017 at 07:55:22PM +0200, Boris Feld wrote:
> > # HG changeset patch
> > # User Boris Feld <boris.feld@octobus.net>
> > # Date 1495197862 -7200
> > #      Fri May 19 14:44:22 2017 +0200
> > # Node ID 985d753d4f5799f2a332140adedb06efd465d62b
> > # Parent  63214f4d9a766761259b650539eede424413e6a2
> > # EXP-Topic obs-cache
> > obsstore: pass a repository object for initialisation
> > 
> > The cache will needs a repository object (to grab a 'vfs'), so we
> > pass a repo object instead of just the 'svfs' and we grab the
> > 'svfs'
> > from there.
> 
> I suspect I'll get to it, but why does this cache want to know about
> anything outside of svfs?
> 
> I'm pretty uncomfortable (architecturally) with passing all of `self`
> into the cache layer.

The obscache need the vfs and not the svfs because caches lives in .hg
and not in .hg/store.

Passing the whole repo and grabbing what we need seemed simpler.

> 
> > 
> > diff -r 63214f4d9a76 -r 985d753d4f57 contrib/perf.py
> > --- a/contrib/perf.py	Fri May 19 14:46:26 2017 +0200
> > +++ b/contrib/perf.py	Fri May 19 14:44:22 2017 +0200
> > @@ -1391,8 +1391,7 @@
> > 
> >      Result is the number of markers in the repo."""
> >      timer, fm = gettimer(ui)
> > -    svfs = getsvfs(repo)
> > -    timer(lambda: len(obsolete.obsstore(svfs)))
> > +    timer(lambda: len(obsolete.obsstore(repo)))
> >      fm.end()
> > 
> >  @command('perflrucachedict', formatteropts +
> > diff -r 63214f4d9a76 -r 985d753d4f57 mercurial/obsolete.py
> > --- a/mercurial/obsolete.py	Fri May 19 14:46:26 2017 +0200
> > +++ b/mercurial/obsolete.py	Fri May 19 14:44:22 2017 +0200
> > @@ -519,10 +519,10 @@
> >      # 1024 byte should be about 10 markers in average
> >      _obskeyspan = 1024
> > 
> > -    def __init__(self, svfs, defaultformat=_fm1version,
> > readonly=False):
> > +    def __init__(self, repo, defaultformat=_fm1version,
> > readonly=False):
> >          # caches for various obsolescence related cache
> >          self.caches = {}
> > -        self.svfs = svfs
> > +        self.svfs = repo.svfs
> >          self._defaultformat = defaultformat
> >          self._readonly = readonly
> > 
> > @@ -799,7 +799,7 @@
> >      if defaultformat is not None:
> >          kwargs['defaultformat'] = defaultformat
> >      readonly = not isenabled(repo, createmarkersopt)
> > -    store = obsstore(repo.svfs, readonly=readonly, **kwargs)
> > +    store = obsstore(repo, readonly=readonly, **kwargs)
> >      if store and readonly:
> >          ui.warn(_('obsolete feature not enabled but %i markers
> > found!\n')
> >                  % len(list(store)))
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Augie Fackler - July 14, 2017, 6:16 p.m.
> On Jul 14, 2017, at 14:15, Boris Feld <boris.feld@octobus.net> wrote:
> 
> On Fri, 2017-07-14 at 14:11 -0400, Augie Fackler wrote:
>> On Sun, Jul 09, 2017 at 07:55:22PM +0200, Boris Feld wrote:
>>> # HG changeset patch
>>> # User Boris Feld <boris.feld@octobus.net>
>>> # Date 1495197862 -7200
>>> #      Fri May 19 14:44:22 2017 +0200
>>> # Node ID 985d753d4f5799f2a332140adedb06efd465d62b
>>> # Parent  63214f4d9a766761259b650539eede424413e6a2
>>> # EXP-Topic obs-cache
>>> obsstore: pass a repository object for initialisation
>>> 
>>> The cache will needs a repository object (to grab a 'vfs'), so we
>>> pass a repo object instead of just the 'svfs' and we grab the
>>> 'svfs'
>>> from there.
>> 
>> I suspect I'll get to it, but why does this cache want to know about
>> anything outside of svfs?
>> 
>> I'm pretty uncomfortable (architecturally) with passing all of `self`
>> into the cache layer.
> 
> The obscache need the vfs and not the svfs because caches lives in .hg
> and not in .hg/store.
> 
> Passing the whole repo and grabbing what we need seemed simpler.

It's simpler today, but more of a potential headache later if someone decides to just retain the whole repo in the cache. I'd rather not do it.
Boris Feld - July 14, 2017, 6:27 p.m.
On Fri, 2017-07-14 at 14:16 -0400, Augie Fackler wrote:
> > On Jul 14, 2017, at 14:15, Boris Feld <boris.feld@octobus.net>
> > wrote:
> > 
> > On Fri, 2017-07-14 at 14:11 -0400, Augie Fackler wrote:
> > > On Sun, Jul 09, 2017 at 07:55:22PM +0200, Boris Feld wrote:
> > > > # HG changeset patch
> > > > # User Boris Feld <boris.feld@octobus.net>
> > > > # Date 1495197862 -7200
> > > > #      Fri May 19 14:44:22 2017 +0200
> > > > # Node ID 985d753d4f5799f2a332140adedb06efd465d62b
> > > > # Parent  63214f4d9a766761259b650539eede424413e6a2
> > > > # EXP-Topic obs-cache
> > > > obsstore: pass a repository object for initialisation
> > > > 
> > > > The cache will needs a repository object (to grab a 'vfs'), so
> > > > we
> > > > pass a repo object instead of just the 'svfs' and we grab the
> > > > 'svfs'
> > > > from there.
> > > 
> > > I suspect I'll get to it, but why does this cache want to know
> > > about
> > > anything outside of svfs?
> > > 
> > > I'm pretty uncomfortable (architecturally) with passing all of
> > > `self`
> > > into the cache layer.
> > 
> > The obscache need the vfs and not the svfs because caches lives in
> > .hg
> > and not in .hg/store.
> > 
> > Passing the whole repo and grabbing what we need seemed simpler.
> 
> It's simpler today, but more of a potential headache later if someone
> decides to just retain the whole repo in the cache. I'd rather not do
> it.

I understand the danger.

Should we pass vfs and svfs explicitly in the V2 then?
Augie Fackler - July 14, 2017, 6:28 p.m.
> On Jul 14, 2017, at 14:27, Boris Feld <boris.feld@octobus.net> wrote:
> 
> On Fri, 2017-07-14 at 14:16 -0400, Augie Fackler wrote:
>>> On Jul 14, 2017, at 14:15, Boris Feld <boris.feld@octobus.net>
>>> wrote:
>>> 
>>> On Fri, 2017-07-14 at 14:11 -0400, Augie Fackler wrote:
>>>> On Sun, Jul 09, 2017 at 07:55:22PM +0200, Boris Feld wrote:
>>>>> # HG changeset patch
>>>>> # User Boris Feld <boris.feld@octobus.net>
>>>>> # Date 1495197862 -7200
>>>>> #      Fri May 19 14:44:22 2017 +0200
>>>>> # Node ID 985d753d4f5799f2a332140adedb06efd465d62b
>>>>> # Parent  63214f4d9a766761259b650539eede424413e6a2
>>>>> # EXP-Topic obs-cache
>>>>> obsstore: pass a repository object for initialisation
>>>>> 
>>>>> The cache will needs a repository object (to grab a 'vfs'), so
>>>>> we
>>>>> pass a repo object instead of just the 'svfs' and we grab the
>>>>> 'svfs'
>>>>> from there.
>>>> 
>>>> I suspect I'll get to it, but why does this cache want to know
>>>> about
>>>> anything outside of svfs?
>>>> 
>>>> I'm pretty uncomfortable (architecturally) with passing all of
>>>> `self`
>>>> into the cache layer.
>>> 
>>> The obscache need the vfs and not the svfs because caches lives in
>>> .hg
>>> and not in .hg/store.
>>> 
>>> Passing the whole repo and grabbing what we need seemed simpler.
>> 
>> It's simpler today, but more of a potential headache later if someone
>> decides to just retain the whole repo in the cache. I'd rather not do
>> it.
> 
> I understand the danger.
> 
> Should we pass vfs and svfs explicitly in the V2 then?

Yep. But let's wait until after the freeze, we've got enough other stuff pouring through the list I don't know that we have time to get through another round on this series.

(If you can do the first two patches and move branch cache to that infrastructure, feel free to send that though)

Patch

diff -r 63214f4d9a76 -r 985d753d4f57 contrib/perf.py
--- a/contrib/perf.py	Fri May 19 14:46:26 2017 +0200
+++ b/contrib/perf.py	Fri May 19 14:44:22 2017 +0200
@@ -1391,8 +1391,7 @@ 
 
     Result is the number of markers in the repo."""
     timer, fm = gettimer(ui)
-    svfs = getsvfs(repo)
-    timer(lambda: len(obsolete.obsstore(svfs)))
+    timer(lambda: len(obsolete.obsstore(repo)))
     fm.end()
 
 @command('perflrucachedict', formatteropts +
diff -r 63214f4d9a76 -r 985d753d4f57 mercurial/obsolete.py
--- a/mercurial/obsolete.py	Fri May 19 14:46:26 2017 +0200
+++ b/mercurial/obsolete.py	Fri May 19 14:44:22 2017 +0200
@@ -519,10 +519,10 @@ 
     # 1024 byte should be about 10 markers in average
     _obskeyspan = 1024
 
-    def __init__(self, svfs, defaultformat=_fm1version, readonly=False):
+    def __init__(self, repo, defaultformat=_fm1version, readonly=False):
         # caches for various obsolescence related cache
         self.caches = {}
-        self.svfs = svfs
+        self.svfs = repo.svfs
         self._defaultformat = defaultformat
         self._readonly = readonly
 
@@ -799,7 +799,7 @@ 
     if defaultformat is not None:
         kwargs['defaultformat'] = defaultformat
     readonly = not isenabled(repo, createmarkersopt)
-    store = obsstore(repo.svfs, readonly=readonly, **kwargs)
+    store = obsstore(repo, readonly=readonly, **kwargs)
     if store and readonly:
         ui.warn(_('obsolete feature not enabled but %i markers found!\n')
                 % len(list(store)))