fix(filesystem): make the walker over a document or archive steerable - #714
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5400f165c6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return std::ranges::any_of(m_files, [&path](const auto &entry) { | ||
| return entry.first.descendant_of(path); |
There was a problem hiding this comment.
When an archive lists a/b.txt before its explicit a/ entry, the file makes is_directory("/a") return true here, so the later create_directory("/a") call returns without adding it. ZipArchive::as_filesystem() inserts entries in archive order, meaning the resulting walker omits /a for this valid ordering and callers cannot encounter or prune that directory with flat_next(). Materialize inferred directories in the walker or otherwise retain explicit directory entries regardless of insertion order.
Useful? React with 👍 / 👎.
`flat_next()`, `pop()` and `depth()` were `// TODO` no-ops on every
document and archive filesystem, while the public header declares them
next to `next()` with no distinction. `flat_next()` was the trap: it did
not advance, so `while (!walker.end()) walker.flat_next();` — the loop it
exists for — never terminated. `depth()` answered 0 for every entry, and
`pop()` left the walker where it stood.
All three follow from the path, which is all the flat map holds. The
walker's own copy of it is ordered component-wise so that an entry's
descendants follow it with nothing in between; ordering the strings
alone does not, since any character below the separator sorts a sibling
into the middle of a subtree ("/a" < "/a-b" < "/a/b"), and both `pop()`
and `flat_next()` skip a subtree by walking past it.
The same map is why `exists()` and `is_directory()` said no to `/` and to
`/Configurations2` while the walker called `/Configurations2/floater` a
directory: an intermediate directory need not be an entry of its own. A
path that entries sit under is now a directory, and so is the root.
Closes #639
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XDs5aK3ZGSZsEvqUUwBBXU
Making `exists()` answer for implied directories also made `create_directory()` refuse one: an archive that names `a/b.txt` before `a/` found `/a` already "existing" and never inserted the entry, so the walk stopped offering `/a` at all and `flat_next()` had nothing to prune. The mutators ask whether there is an entry of that name, which is the question they were always asking; `exists()` and `is_directory()` keep answering for what the filesystem implies, which is what the walker reports. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XDs5aK3ZGSZsEvqUUwBBXU
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015d5RcmsA777vwXiuafjx6k
13b643a to
528207c
Compare
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015d5RcmsA777vwXiuafjx6k
1 participant
🤖 Generated with Claude Code
Closes #639.
VirtualFileWalker::flat_next(),pop()anddepth()were// TODOno-ops — on every document and every archive filesystem, i.e. everythingDocument::as_filesystem()andArchive::as_filesystem()hand out, and reachable from every binding.src/odr/filesystem.hppdeclares them besidenext()with no distinction, so a caller had no way to know.flat_next()was the trap: it did not advance, so the loop it exists for never terminated.What they do now
All three follow from the path alone, which is all the flat map holds:
depth()— components below the walked root,0for an entry sitting directly in it, likestd::filesystem::recursive_directory_iterator.flat_next()— the next entry that is not under the current one.pop()— leaves the directory the current entry sits in. At depth 0 that is the end, again matchingrecursive_directory_iterator.Both skips walk past a subtree, which needs the subtree to be contiguous — so the walker's own copy of the map is ordered component-wise rather than by string. Ordering the strings alone does not do it: any character below the separator sorts a sibling into the middle of a subtree,
"/a" < "/a-b" < "/a/b". There is a test for exactly that shape. The visit order is unchanged for every path set that does not contain one.The second half of the issue
The same "only explicit map entries exist" is why
exists()andis_directory()returnedfalsefor/and for/Configurations2, while the walker reported/Configurations2/floateras a directory. An intermediate directory need not be an entry of its own — a zip may name only its files — so a path that entries sit under is now a directory too, and so is the root. That makesFilesystemagree with its own walker.Verified
Eight new tests in
test/src/internal/common/filesystem_test.cpp, built on hand-madeVirtualFilesystems (no fixtures), covering each method, the sibling-sorts-into-the-subtree case, and the directory visibility:Full suite: 948 passed, 8 skipped, 1 failed —
odr_public_pdf_opendocument_app_website_pdf, which fails identically onmainand is unrelated.