[PATCH] localrepo: clear the filecache on _rollback() and destroyed()

[Greg Ward]

[me]
> It's unclear what you're fixing here.

[Idan Kamara] 
> I'm fixing this: http://selenic.com/repo/hg/rev/357e6bcfb619 which
> basically disabled
> the filecache.

So, this is a performance fix? And that's why there is no change to
the tests? Or did 357e6bcfb619 affect correctness?

[back to the patch]
> # HG changeset patch
> # User Idan Kamara <idankk86 at gmail.com>
> # Date 1343504430 -10800
> # Branch stable
> # Node ID b4aefebff45caa66ca253c43a673b5847a2b84a6
> # Parent  a09cc6aeed4a6d54a3846b6653165fd3b328d1ef
> localrepo: clear the filecache on _rollback() and destroyed()
>
> This restores the old behaviour of clearing the filecache when the repo is
> destroyed but combines it with also clearing it on _rollback. Before, we tried
> to only call it through _rollback but that ruined callers of destroyed.
>
> Doing it on both code paths covers destroyed being called from somewhere
> else, e.g. strip.

You're essentially backing out 357e6bcfb619 here, so IMHO you should
mention it in the commit message. From Matt's commit message there, it
sounds like *he* was fixing something, so you're in danger of getting
into patch ping-pong. Nobody wins that game.

OK, I have reviewed the last couple of patches in this area
(ce0ad184f489, 9a99224a6409, 357e6bcfb619), and I *think* your patch
is correct, although it's too bad that we have to call
self._filecache.clear() from two disparate places. (And I can see that
you and Matt are already playing patch ping-pong. Good luck with
that.)

But I can just see some enterprising young developer coming along and
"cleaning things up", causing a performance regression -- again. (I'm
still assuming all the bugs here are performance, not correctness.)

So... is there any way to test it?

       Greg
-- 
Greg Ward                                http://www.gerg.ca/
Know thyself.  If you need help, call the CIA.