[PATCH 1 of 5 STABLE] filecache: pass func name that constructs the full path of the given file

[Idan Kamara]

On Wed, Feb 29, 2012 at 12:59 AM, Matt Mackall <mpm at selenic.com> wrote:

> On Wed, 2012-02-29 at 00:21 +0200, Idan Kamara wrote:
> > On Wed, Feb 29, 2012 at 12:03 AM, Matt Mackall <mpm at selenic.com> wrote:
> >
> > > On Tue, 2012-02-28 at 17:53 +0200, Idan Kamara wrote:
> > > > # HG changeset patch
> > > > # User Idan Kamara <idankk86 at gmail.com>
> > > > # Date 1330444187 -7200
> > > > # Branch stable
> > > > # Node ID 94da74854f8123af5f81c1a996352fdaeffb23d5
> > > > # Parent  de7aec5079bdaaae8509ec74a57fc2cb2c160371
> > > > filecache: pass func name that constructs the full path of the given
> file
> > > >
> > > > diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> > > > --- a/mercurial/localrepo.py
> > > > +++ b/mercurial/localrepo.py
> > > > @@ -176,7 +176,7 @@
> > > >      def _writebookmarks(self, marks):
> > > >        bookmarks.write(self)
> > > >
> > > > -    @filecache('phaseroots', True)
> > > > +    @filecache('phaseroots', 'sjoin')
> > >
> > > Err.. yuck. We're going to pass a string and then look it up with
> > > getattr?
> > >
> > > Ok, so now I'm at "yuck", but I have no idea what the motivation for
> > > this is without reading all the patches. But all the patches have
> > > one-line descriptions so I have no idea what the underlying bug is and
> > > why this is the right fix. So now I'm at "yuck" plus "huh?".
> > >
> >
> > Yes, perhaps I should have included a summary for this
> > series.
>
> For the record, the most recent
> everybody-please-read-this-stop-sending-summaries message was this one
> last week:
>
> http://mercurial.markmail.org/thread/bhsyg5elj7daotcd
>
> Just write a proper patch description, please.
>
> > > My take: the deeper problem here is that the filecache scheme is
> > > incestuous with the repo object, which makes it hard to repurpose for
> > > use with dirstate. So it seems the right fix is to have a base class
> > > (generic file caching) and derived classes (current class + one to make
> > > dirstate happy).
> >
> >
> > Hm, that sounds like overkill to the problem this was meant to fix.
> >
> > Currently filecache is coupled with localrepo because it assumes
> > the files it monitors are either in .hg or .hg/store, this is controlled
> > by the instore flag which tells it which of (s)join methods to use when
> > figuring out the path of the monitored file at runtime.
> >
> > So now dirstate comes along and that doesn't quite fit because it
> > uses different names and it also caches files in the repository root.
> >
> > But all we really care about is how to compute the final path, so
> > passing the function seems like the right thing to do, rather than two
> > classes (or more when filecache is needed in another class) with
> hardcoded
> > function calls.
>
> But you're not passing "the function", you're passing a string. And a
> string is a different conceptual object: the compiler won't tell you
> when you're doing it wrong. And in that sense, it is just as "hardcoded"
> as making a derived class. Except the derived class hardcodes it in one
> place, rather than in every use.
>

It might be conceptually different but practically it doesn't matter here.
Everything happens in runtime, the compiler won't tell us anything and
we'll get the same error at the same time in both implementations.


>
> Making this work cleanly is a simple matter of localizing the
> path-joining logic to a single method that the derived class overrides:
>
> class storecache(scmutil.filecache):
>    def join(self, obj, fname):
>        return obj.sjoin(fname)


I guess that's better, except it forces all clients of such classes
to name their join function the same.

So we'll have repocache -> obj.join (need to add a 'join' function
to dirstate). And rootcache -> obj.rjoin?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20120229/eeca0a06/attachment.html>