Tag caching, at last

[Matt Mackall]

On Sat, 2009-07-11 at 22:01 +0200, Dirkjan Ochtman wrote:
> On Fri, Jul 10, 2009 at 17:34, Greg Ward<greg at gerg.ca> wrote:
> > Please let me know what you think.  I'm not convinced that having the
> > tag cache in another class is necessary, but it's been very handy for
> > me staying sane while juggling multiple implementations.  It also
> > leaves us the possibility of leaving in a 'nulltagcache' class, so
> > that you can easily revert to 1.3 behaviour if you see tag caching
> > bugs between now and 1.4.
> 
> I'm +1 on the separate class, if only on the grounds that localrepo is
> big enough as it is and separating out some clear unit of behavior
> would seem to be a good idea. I also think starting with
> just-the-filenodes caching is a good idea as a starting point. We can
> always go the final step if we find we need it and it's not too
> complex.

I don't see much point in this being a class. A class is an abstraction
that makes manipulating complex data simpler. If you don't have some
combination at least one if not all three of a) numerous instances b)
numerous members or c) numerous methods, it probably only makes things
more complex to make something a class. 

If the goal is to simplify localrepo, then it's probably time to make a
whole new module. Then we'll probably find that:

 self._tags = tags.readtags(ui, repo)

is actually easier to do than:

 t = tags.tagcache(ui, repo)
 self._tags = t.readtags()

on both the caller and callee side.

Please, don't reach for a class as the first answer to every problem.
Some things are better left as plain old functions.

-- 
http://selenic.com : development and support for Mercurial and Linux