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

Matt Mackall mpm at selenic.com
Tue Feb 28 16:59:06 CST 2012


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.

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)

-- 
Mathematics is the supreme nostalgia of our time.




More information about the Mercurial-devel mailing list