Skip to content
This repository has been archived by the owner on Nov 3, 2023. It is now read-only.

Repository class caches trees forever #29

Open
danports opened this issue Apr 19, 2020 · 6 comments · May be fixed by #32
Open

Repository class caches trees forever #29

danports opened this issue Apr 19, 2020 · 6 comments · May be fixed by #32

Comments

@danports
Copy link
Contributor

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 danports/amber#14)

@eric-wieser
Copy link
Owner

Which cache are you referring to?

@danports
Copy link
Contributor Author

if not __repoPriv[self].trees[sha] then

@eric-wieser
Copy link
Owner

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

@danports
Copy link
Contributor Author

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

@danports
Copy link
Contributor Author

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:

local __repoPriv = setmetatable({}, {mode='k'})

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

@danports
Copy link
Contributor Author

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. 😕

danports added a commit to danports/computercraft-github that referenced this issue Apr 20, 2020
danports added a commit to danports/computercraft-github that referenced this issue Apr 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants