feat!: mount command - #1
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe change adds LZR2 and BWTM2 compression paths, version 2 segmented containers, lazy range decoding, FDR2 directory CRC metadata, Unix FUSE mounting, new CLI commands, updated completions, documentation, benchmarks, CI setup, and tests. ChangesArchive and compression pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant FossilFs
participant LazyContainer
participant FUSE
participant Archive
CLI->>FossilFs: mount archive at mountpoint
FossilFs->>LazyContainer: parse archive lazily
FUSE->>FossilFs: request file read or mutation
FossilFs->>LazyContainer: decode requested range
FossilFs->>Archive: repack modified filesystem on unmount
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
289-292: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winKeep the documented FDR2 manifest contract consistent.
The supplied writer stores each file’s path, length, and CRC32; offsets are derived cumulatively rather than serialized.
README.md#L289-L292: replace “manifest of each file’s offset and CRC32” with wording that includes path, length, and CRC32.docs/src/pages/index.astro#L289-L292: make the same correction in the site’s format description.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 289 - 292, Update the FDR2 manifest descriptions at README.md lines 289-292 and docs/src/pages/index.astro lines 289-292 to state that each file records its path, length, and CRC32; remove the claim that offsets are included in the manifest, since offsets are derived cumulatively.
🧹 Nitpick comments (2)
tests/container.rs (1)
88-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEnsure this test actually exercises v2 block framing.
The version-byte assertion does not prove
writeselectedMODE_BLOCKS; stored form can satisfy the round-trip and lazy-range assertions. Assert thatc.blocks.len() > 1or construct the v2 framing explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/container.rs` around lines 88 - 97, Update v2_framing_round_trips_multi_block to verify that the decoded container’s blocks collection contains more than one block, ensuring write selected MODE_BLOCKS; retain the existing version, round-trip, and lazy-range assertions.src/core/models/lz.rs (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider asserting
HISTORYmatches the segment size it's designed for.
HISTORY(262144) now exactly matchesSEGMENT_BLOCKS * BLOCK_SIZEused for directory segmentation (percontainer.rs/block.rscontext), lettingencode_block_seguse a full segment as back-reference window. This coupling across files is implicit and unenforced — if segment sizing constants change later, compression quality silently regresses without any test failure.A compile-time assertion (
const { assert!(...) }orconst _: () = assert!(...), stable since Rust 1.57/1.79) tying the two constants together would catch drift immediately.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/models/lz.rs` at line 6, Update the constants near HISTORY in the lz model to add a compile-time assertion that HISTORY equals the directory segment size computed from SEGMENT_BLOCKS and BLOCK_SIZE. Reference the existing constants from container.rs/block.rs as appropriate, preserving HISTORY’s current value while making future sizing drift fail during compilation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bench.sh`:
- Around line 53-54: Update the benchmark archive handling around the "$BIN"
pack invocation to create a unique temporary directory with mktemp -d, register
a trap to remove it on exit, and write/read the archive within that directory
instead of the predictable /tmp/bench.fossil path. Make the script exit
immediately when pack fails so it cannot measure a stale archive.
In `@src/commands/mount.rs`:
- Around line 584-602: The mkdir method reports success for directories that are
not persisted. Update mkdir and the manifest/dir::pack serialization path so
newly created empty directories are recorded and restored; otherwise return
libc::EOPNOTSUPP instead of replying with a successful entry. Ensure the chosen
behavior prevents successful directory creation from disappearing after unmount.
- Around line 428-439: Bound file growth before calling Vec::resize in the
size-handling path around materialize and the corresponding sparse-write path
near the second occurrence. Validate the requested length conversion and enforce
the project’s maximum supported dense-buffer size; reject oversized or invalid
values with an appropriate filesystem error, preserving existing behavior for
safe lengths and avoiding allocation before validation.
- Around line 782-790: Update the fsync method to persist any dirty archive
state before replying success, by invoking the existing repack or durable-write
mechanism; if persistence fails, return an appropriate failure through
ReplyEmpty instead of acknowledging the sync.
- Around line 410-426: Update setattr so requests containing unsupported mode,
uid, gid, or timestamp fields return EOPNOTSUPP instead of reporting success;
continue applying size-only changes through the existing setattr flow. Use the
corresponding optional parameters in setattr, including _atime, _mtime, _ctime,
_crtime, and _chgtime, while preserving success for requests that do not ask for
unsupported attribute updates.
- Around line 664-705: Update rename to honor _flags by rejecting an existing
destination with RENAME_NOREPLACE, and validate the destination before mutating
either namespace. When replacing an existing destination, reject non-empty
directories and perform type-compatible checks before removing the node;
preserve descendants and namespace state on all rejected operations. Use the
existing child, nodes, and Kind::Dir handling in rename.
- Around line 342-365: Update collect and its caller repack so materialize
failures are propagated instead of skipped: change collect to return an
io::Result, return the materialization error when processing a file fails, and
propagate recursive collect errors. Ensure repack handles the collect failure
and does not write a replacement archive when any file cannot be materialized.
- Around line 875-888: Update force_unmount so the libc::umount2 call is
compiled only for Linux/Android targets, and add appropriate per-OS handling for
other supported Unix targets such as FreeBSD and Solaris if they require
different APIs. Keep the existing macOS libc::unmount path unchanged.
In `@src/commands/take.rs`:
- Around line 47-92: Update the directory extraction branch in the `take` flow
so `trust == false` still verifies integrity when `want_crc` is `None` for
legacy FDIR manifests. Reuse the existing full-archive CRC verification path, or
reject extraction with an appropriate error unless `--trust` is enabled; retain
the per-file checksum validation when `want_crc` is present.
In `@src/core/container.rs`:
- Around line 289-301: Validate the v2 last-block length immediately after
reading last_len in both parser paths, whenever n_blocks > 0: reject zero and
values greater than BLOCK_SIZE before using it as an allocation or decoding
length. Apply the same validation in the parsing logic around the shown block
and the corresponding parser near the alternate location, preserving existing
behavior for non-v2 archives and empty block lists.
- Around line 476-490: Update Container::read_range to account for self.filter
instead of returning decode_range’s transformed bytes directly. Implement
filtered range reconstruction so lazy reads return the original byte range, or
explicitly reject filtered-container calls with an appropriate io::Error;
preserve the existing bounds validation and unfiltered decode_range behavior.
In `@src/core/models/bwtm.rs`:
- Around line 30-43: Bound entropy-coded base-128 decoding in decode_run: cap
the number of chunks before shifting, and reject any decoded run that exceeds
the remaining orig_len before using it. In src/core/models/lzr.rs lines 147-164,
apply the same chunk limit and validate decoded lengths against the remaining
target and distances against available history; update the relevant LZR decoding
symbols without changing valid-input behavior.
In `@src/main.rs`:
- Around line 301-302: Update the command-suggestion list in the main command
handling flow to include "mount" only on Unix targets, matching the existing
target gate used by command dispatch and help. Keep "mount" available in
suggestions on Unix builds while excluding it from non-Unix builds.
---
Outside diff comments:
In `@README.md`:
- Around line 289-292: Update the FDR2 manifest descriptions at README.md lines
289-292 and docs/src/pages/index.astro lines 289-292 to state that each file
records its path, length, and CRC32; remove the claim that offsets are included
in the manifest, since offsets are derived cumulatively.
---
Nitpick comments:
In `@src/core/models/lz.rs`:
- Line 6: Update the constants near HISTORY in the lz model to add a
compile-time assertion that HISTORY equals the directory segment size computed
from SEGMENT_BLOCKS and BLOCK_SIZE. Reference the existing constants from
container.rs/block.rs as appropriate, preserving HISTORY’s current value while
making future sizing drift fail during compilation.
In `@tests/container.rs`:
- Around line 88-97: Update v2_framing_round_trips_multi_block to verify that
the decoded container’s blocks collection contains more than one block, ensuring
write selected MODE_BLOCKS; retain the existing version, round-trip, and
lazy-range assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6c8b15d9-e29a-41b3-baab-c406be902340
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
BENCHMARK.mdCargo.tomlREADME.mdbench.shdocs/src/pages/index.astroshare/fossil.1share/fossil.bashshare/fossil.fishshare/fossil.zshsrc/commands/explain.rssrc/commands/map.rssrc/commands/mod.rssrc/commands/mount.rssrc/commands/take.rssrc/core/block.rssrc/core/container.rssrc/core/dir.rssrc/core/models/bwtm.rssrc/core/models/huffman.rssrc/core/models/lz.rssrc/core/models/lzr.rssrc/main.rstests/container.rstests/dir.rstests/lazy.rstests/models/bwtm.rstests/models/huffman.rstests/models/lzr.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/commands/unpack.rs (1)
139-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate unsafe-path check — extract a shared helper.
The
Component::ParentDir | Component::RootDir | Component::Prefix(_)filtering closure is copy-pasted from the file-extraction loop above (Lines 106-111) into this new dirs loop.♻️ Proposed refactor
+fn is_unsafe_rel_path(rel_path: &Path) -> bool { + rel_path.components().any(|part| { + matches!( + part, + Component::ParentDir | Component::RootDir | Component::Prefix(_) + ) + }) +} + fn unpack(input: &str, output: &str, trust: bool) -> io::Result<UnpackReport> {Then use
is_unsafe_rel_path(rel_path)in both the file loop and this dirs loop instead of the inline closures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/unpack.rs` around lines 139 - 155, Extract the repeated path-component filtering logic into a shared is_unsafe_rel_path helper, then replace the inline closures in both the file-extraction loop and the directory loop with calls to that helper. Preserve the existing handling that skips unsafe paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/pack.rs`:
- Around line 204-237: Update collect_files so every empty directory, including
the root input directory, is added to dirs; remove the dir != base condition
from the entries.is_empty() check. Preserve rel_path’s empty string for the root
so unpacking recreates the destination root through the existing dirs loop.
---
Nitpick comments:
In `@src/commands/unpack.rs`:
- Around line 139-155: Extract the repeated path-component filtering logic into
a shared is_unsafe_rel_path helper, then replace the inline closures in both the
file-extraction loop and the directory loop with calls to that helper. Preserve
the existing handling that skips unsafe paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2499ea11-002f-4028-87b5-94f4110aad91
📒 Files selected for processing (10)
bench.shsrc/commands/mount.rssrc/commands/pack.rssrc/commands/take.rssrc/commands/unpack.rssrc/core/container.rssrc/core/dir.rssrc/core/models/bwtm.rssrc/core/models/lzr.rssrc/main.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- bench.sh
- src/core/models/lzr.rs
- src/core/dir.rs
- src/main.rs
- src/core/models/bwtm.rs
- src/commands/mount.rs
- src/commands/take.rs
- src/core/container.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Cargo.toml (1)
9-11: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winVerify that
mountshould be enabled by default.
default = ["mount"]makes the new FUSE path part of every normal Unix build. If the objective is to keep mount dependencies conditional and opt-in, use an empty default feature set and require--features mount; otherwise document this compatibility change. Cargo enables features listed underdefaultfor ordinary builds. (doc.rust-lang.org)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Cargo.toml` around lines 9 - 11, Update the [features] configuration so mount dependencies remain opt-in by default: remove "mount" from the default feature set and retain the existing mount feature definition for builds explicitly using --features mount.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Cargo.toml`:
- Around line 9-11: Update the [features] configuration so mount dependencies
remain opt-in by default: remove "mount" from the default feature set and retain
the existing mount feature definition for builds explicitly using --features
mount.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f00d2bc5-ac55-4d41-bfdc-a832b5d2bcf7
📒 Files selected for processing (10)
.github/workflows/ci.yml.github/workflows/release.ymlCargo.tomlbuild.rssrc/commands/mod.rssrc/commands/mount.rssrc/commands/pack.rssrc/core/models/lz.rssrc/main.rstests/container.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- src/commands/mod.rs
- src/core/models/lz.rs
- src/commands/pack.rs
- src/main.rs
- tests/container.rs
- src/commands/mount.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
add new mount command (note this is still v2 and breaks some exisitng dir encoding)
Summary by CodeRabbit
fossil list,fossil take, and Unix-onlyfossil mount(writable filesystem) commands.take --trustto skip verification;mountalso supports--verbose/--log.