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

Fetch tree and blob contents lazily #27

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

danports
Copy link
Contributor

Closes #26.

This PR necessarily introduces a small breaking change - Tree.size no longer represents the total size of all files in the tree, only the total size of the files at the top level. Tree:getFullSize is introduced to return what Tree.size originally did.

In addition to Blob:getContents and Tree:getContents, a convenience method Tree:getChild has also been added.

apis/github Outdated
Comment on lines 100 to 101
if self.contents then
return self.contents
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this look more private?

At risk of revealing that I now develop python and not lua, perhaps

Suggested change
if self.contents then
return self.contents
if self._contents then
return self._contents

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, will this fail for empty files? Should it be contents ~= nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, private change committed in 86fb962.

That test will work fine for empty directories - an empty table is a truthy value, unlike nil.

@danports
Copy link
Contributor Author

Discovered a couple of issues with this PR in testing, will push an update later.

@danports danports marked this pull request as draft April 13, 2020 17:18
@danports
Copy link
Contributor Author

Fixed a few issues with this PR:

  • The old Tree:iter coroutine-based implementation was elegant, but it doesn't play nice with any other code that calls (directly or indirectly) os.pullEvent during iteration. os.pullEvent uses coroutines in a very specific way, and http.get is one of those functions that uses os.pullEvent internally, so if you try to call Blob:getContents or some of the other functions here that use http.get while Tree:iter is running (while its coroutine is suspended, technically), http.get will call os.pullEvent and ends up resuming Tree:iter and actually losing the HTTP response. You get very strange and difficult-to-debug behavior, which is likely not what the user of Tree:iter is expecting. So I replaced the coroutine-based implementation with a simple implementation that uses a table as a call stack.
  • raw.github.com URLs need a commit SHA rather than a blob SHA.
  • The github client program needs to use the new Tree:getFullSize method.

Tested and verified the github client against a simple repository - everything is working as expected.

@danports danports marked this pull request as ready for review April 19, 2020 19:39
@eric-wieser
Copy link
Owner

I'm in danger of being nerd-sniped by your is.pullEvent remark, having been involved in writing coroutine schedulers in Python. It's been a while since I've written Lua though...

@eric-wieser
Copy link
Owner

eric-wieser commented Apr 19, 2020

Perhaps something simple like:

sentinel = {}
yield = function(...)
    coroutine.yield(sentinel, ...)
end
wrap = function(coro)
    native_wrap = coroutine.wrap(coro)
    return function(...)
        args = pack(...)
        while true do
            ret = pack(native_wrap(unpack(args))
            if ret[1] == sentinel then
                return unpack(ret, 1)
            end
            args = pack(coroutine.yield(unpack(ret)))
        end
    end
end

@danports
Copy link
Contributor Author

It's been a while since I read through the ComputerCraft BIOS/CraftOS internals, so I'm not sure if that would solve the problem...all I recall now is that coroutines (outside of "normal" event loops) were the problem and not using them solved it. 😆 As the ComputerCraft documentation says:

If you choose to use coroutines in a different way, then os.pullEventRaw and os.pullEvent will not work. This in turn will break any other API function which is written on top of os.pullEvent.

@eric-wieser
Copy link
Owner

The idea behind the pseudocode above is to distinguish our yields from theirs, and propagate theirs when they occur. It's definitely not correct as written, I forget how varargs work.

@danports
Copy link
Contributor Author

Let's assume the pseudocode works. Do you think it's worth "fixing" ComputerCraft's coroutines in this case, since they're only used in one (relatively minor) place in the library? I agree that it's an interesting intellectual exercise, but I'm not sure it's worth it in this case - I think the simplest solution is probably best.

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 this pull request may close these issues.

Fetch tree and blob contents lazily
2 participants