Conversation
petersalomonsen
left a comment
There was a problem hiding this comment.
Thanks for the PR, and for the clear write-up in #125 — the use case makes sense and I'd like to support it.
I built this locally against the pinned Emscripten 6.0.3 and libgit2 1.9.4: ./build.sh Release-workerfs succeeds and all three of the new tests pass. Three things I'd like to sort out before merging.
1. This probably doesn't need to be a separate variant
Same toolchain, same tree, three builds:
| build | .js |
.wasm |
wasm md5 |
|---|---|---|---|
default Release |
110,554 | 863,217 | 819dc3b0416c7973f4f994017f13c04f |
this PR's Release-workerfs |
99,803 | 863,217 | 819dc3b0416c7973f4f994017f13c04f |
default Release + -lworkerfs.js |
113,554 | 863,217 | 819dc3b0416c7973f4f994017f13c04f |
The .wasm is byte-identical in all three — the FS backends are pure JS glue, so the entire variant is a difference in the generated JS. Adding -lworkerfs.js to the existing sync build and 'WORKERFS' to its EXPORTED_RUNTIME_METHODS costs +3,000 bytes of JS and zero wasm. I ran your new test suite unchanged against that build and all three tests pass.
As it stands the PR adds another 863 KB of duplicate wasm to the npm tarball, a seventh variant to document, and a full CI job — and because Release-workerfs links only -lworkerfs.js -lmemfs.js, it also means there is no build with both WORKERFS and IDBFS, so anyone wanting a WORKERFS mount and IndexedDB persistence is stuck.
Would you be up for folding WORKERFS into the default sync build instead and dropping the variant, the CI job, and the extra package files?
2. Leaked index in lg2.c
git_repository_set_index() takes its own reference — set_index() does GIT_REFCOUNT_INC(index) — so the caller has to release its own. libgit2's own test_repo_setters__setting_a_new_index_on_a_repo_which_has_already_loaded_one_properly_honors_the_refcount asserts refcount 2 after the call and then frees, and git_index_open()'s docs say "The index must be freed once it's no longer in use."
So this needs a git_index_free(index); after the git_repository_set_index() call. It matters here more than in a normal CLI, since in a worker callMain() is invoked repeatedly against a long-lived module and each call would leak an index plus its entries.
3. status / diff against a WORKERFS mount don't currently work
The two new tests are each half of the story — one mounts a blob and reads it back, the other runs add on a MEMFS repo — but neither runs a git command against a WORKERFS-mounted repository, which is the actual use case. I wrote that end-to-end test locally: commit a repo in MEMFS, re-mount a copy of it through WORKERFS using files (real File objects, so mtimes are preserved exactly — I verified that), then run git against the mount.
log and the other history commands work fine. But status and diff report every tracked file as modified, on a pristine checkout:
diff --git a/tracked.txt b/tracked.txt
old mode 100644
new mode 100755
WORKERFS hardcodes FILE_MODE = S_IFREG | 0o777, so every file stats as 0777 and libgit2 compares 100755 against the 100644 in the index. I couldn't configure it away: core.filemode = false in the global /home/web_user/.gitconfig has no effect, because the repo's own .git/config (filemode = true, as git init writes and as any real repo on Linux/macOS will have) lives on the read-only mount and takes precedence. Pointing --git-dir at a writable MEMFS copy of .git with the flag flipped breaks differently — the worktree association is lost and everything turns into deleted file mode 100644.
Separately, --index-file pointed at a path that doesn't exist yet gives you an empty index, so status reports every file as deleted:. To get sensible output you first have to copy the repo's existing .git/index to the writable path, which the README section doesn't mention.
So I think the README wording — "Redirect the git index to MEMFS with lg2's --index-file flag ... so index-writing commands work against a read-only repository mount" — promises more than currently holds. Could you add an end-to-end test that actually mounts a repo through WORKERFS and runs git against it? That's the test that surfaces the filemode problem, and I'd rather we either solve it or document the limitation honestly than ship a variant whose headline feature only half works.
None of this is a criticism of the direction — I want WORKERFS support in. Happy to take a smaller PR that just adds -lworkerfs.js + the WORKERFS export to the default build with a test, and treat the --index-file flag and the filemode question as a follow-up if that's easier to land.
Closes #125