<div dir="ltr"><div class="gmail_quote">On Wed, Feb 29, 2012 at 12:03 AM, Matt Mackall <span dir="ltr"><<a href="mailto:mpm@selenic.com">mpm@selenic.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">On Tue, 2012-02-28 at 17:53 +0200, Idan Kamara wrote:<br>
> # HG changeset patch<br>
> # User Idan Kamara <<a href="mailto:idankk86@gmail.com">idankk86@gmail.com</a>><br>
> # Date 1330444187 -7200<br>
> # Branch stable<br>
> # Node ID 94da74854f8123af5f81c1a996352fdaeffb23d5<br>
> # Parent  de7aec5079bdaaae8509ec74a57fc2cb2c160371<br>
> filecache: pass func name that constructs the full path of the given file<br>
><br>
> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py<br>
> --- a/mercurial/localrepo.py<br>
> +++ b/mercurial/localrepo.py<br>
> @@ -176,7 +176,7 @@<br>
>      def _writebookmarks(self, marks):<br>
>        bookmarks.write(self)<br>
><br>
> -    @filecache('phaseroots', True)<br>
> +    @filecache('phaseroots', 'sjoin')<br>
<br>
</div>Err.. yuck. We're going to pass a string and then look it up with<br>
getattr?<br>
<br>
Ok, so now I'm at "yuck", but I have no idea what the motivation for<br>
this is without reading all the patches. But all the patches have<br>
one-line descriptions so I have no idea what the underlying bug is and<br>
why this is the right fix. So now I'm at "yuck" plus "huh?".<br></blockquote><div><br></div><div>Yes, perhaps I should have included a summary for this</div><div>series.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<br>
My take: the deeper problem here is that the filecache scheme is<br>
incestuous with the repo object, which makes it hard to repurpose for<br>
use with dirstate. So it seems the right fix is to have a base class<br>
(generic file caching) and derived classes (current class + one to make<br>
dirstate happy).</blockquote><div><br></div><div>Hm, that sounds like overkill to the problem this was meant to fix.</div><div><br></div><div>Currently filecache is coupled with localrepo because it assumes</div><div>the files it monitors are either in .hg or .hg/store, this is controlled</div>
<div>by the instore flag which tells it which of (s)join methods to use when</div><div>figuring out the path of the monitored file at runtime.</div><div><br></div><div>So now dirstate comes along and that doesn't quite fit because it</div>
<div>uses different names and it also caches files in the repository root.</div><div><br></div><div>But all we really care about is how to compute the final path, so</div><div>passing the function seems like the right thing to do, rather than two</div>
<div>classes (or more when filecache is needed in another class) with hardcoded</div><div>function calls.</div></div></div>