eric-wieser / computercraft-github

A readonly github repository client for computercraft
MIT License
27 stars 15 forks source link

Repository class caches trees forever #29

Open danports opened 4 years ago

danports commented 4 years ago

Forever is a long time. There should be an automatic or manual way to flush the Repository.tree cache. Options:

  1. Check the SHA of the provided commit-ish on GitHub. If it matches the cached SHA, cool; otherwise flush the cache and instantiate a new Tree.
  2. Add a method on Repository to remove the cache entry for a particular commit-ish (or all cache entries).

I like the first option better. If one API call is too much of a perf hit for Repository.tree, we could make the staleness check optional with a parameter. Thoughts?

(Needed for https://github.com/danports/amber/issues/14)

eric-wieser commented 4 years ago

Which cache are you referring to?

danports commented 4 years ago

https://github.com/eric-wieser/computercraft-github/blob/208033bc4d3cbfc1be2bc6d0fe1eabab69a07dc5/apis/github#L179

eric-wieser commented 4 years ago

I assume the issue is when sha is not a sha at all, but a named ref?

danports commented 4 years ago

Right - in some ComputerCraft setups, that cache can live for a long time, and master can change frequently.

danports commented 4 years ago

One issue I see is that the cache itself is supposed to be using weak references for the keys (Repository instances) but defines the metatable incorrectly - according to this, it should be __mode, not mode: https://github.com/eric-wieser/computercraft-github/blob/208033bc4d3cbfc1be2bc6d0fe1eabab69a07dc5/apis/github#L169

But changing that wouldn't resolve this issue anyway, since a caller might reasonably keep a Repository reference around for a while.

danports commented 4 years ago

I also don't understand why that cache is defined the way it is. If you're going to have a table with weak references for keys, why not just make the tree SHA table a private field on the Repository instance? Why bother having a global table at all? The only reason I could see for a global table would be to try to reuse tree SHA caches across different Repository instances with the same properties (user, name, etc.), but that isn't happening right now. 😕