Refactor: use paths.rs helpers instead of inline .quilt literals#698
Open
fiskus wants to merge 5 commits into
Open
Refactor: use paths.rs helpers instead of inline .quilt literals#698fiskus wants to merge 5 commits into
.quilt literals#698fiskus wants to merge 5 commits into
Conversation
Expose `DOT_QUILT_DIR` and `DomainPaths::dot_quilt_dir()` so callers don't need to spell `.quilt` (or its subpaths) by hand. - diagnostics.rs: lineage zip entry uses `DomainPaths::lineage()`. - commands.rs: debug-open uses `DomainPaths::dot_quilt_dir()`. - fswatcher/filter.rs: ignore check uses `DOT_QUILT_DIR`. - install.rs tests: post-install assertions use `installed_manifest`, `cached_manifests_dir`, and `objects_dir` instead of joined string literals.
After the previous commit, downstream tests use `DomainPaths` helpers instead of inline `.quilt/...` strings — so a typo in `LINEAGE_FILE` or `OBJECTS_DIR` would silently round-trip through every consumer. Add one test that asserts the byte-exact path each accessor returns, giving paths.rs an independent witness for its own constants.
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
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.
Summary
DOT_QUILT_DIRandDomainPaths::dot_quilt_dir()inquilt-rs::paths, and replace inline.quiltliterals with helper calls inquilt-sync(telemetry::diagnostics,commands::debug_dot_quilt,fswatcher::filter) and thequilt-cliinstall test.test_domain_paths_layoutto pin the byte-exact path eachDomainPathsaccessor returns, so any drift inDOT_QUILT_DIR/LINEAGE_FILE/INSTALLED_DIR/MANIFEST_DIR/OBJECTS_DIR/AUTH_DIRfails one focused test rather than silently round-tripping through every downstream caller.Context
Item #3 from
plans/cli/shared-data-dir/problem-rethinking.md: the.quiltdirectory structure was duplicated as inline strings across crates despite a sharedpathsmodule. This PR consolidates the easy cases. Items #1, #2, #4, #5 from that doc are out of scope.No user-visible behavior change → no
CHANGELOG.mdentry, no alpha bump.Test plan
cargo check --workspace --all-targets— clean (the pre-existing clippy warning incommands.rs:3245reproduces onmain)cargo test -p quilt-rs --lib paths— 11 passed including newtest_domain_paths_layoutcargo test -p quilt-sync telemetry::diagnostics— 3 passedcargo test -p quilt-sync fswatcher::filter— 4 passedquilt-cliinstall tests (require S3 access) — compile-checked viacargo test -p quilt-cli --no-runGreptile Summary
This PR consolidates inline
".quilt"string literals scattered acrossquilt-syncandquilt-clibehind a newDOT_QUILT_DIRconstant andDomainPaths::dot_quilt_dir()accessor inquilt-rs::paths, with no user-visible behavior change.pub const DOT_QUILT_DIRandDomainPaths::dot_quilt_dir()toquilt-rs/src/paths.rs, and updates four call-sites incommands.rs,fswatcher/filter.rs,diagnostics.rs, and thequilt-cliinstall test to use these helpers instead of raw string literals.test_domain_paths_layoutto pin the byte-exact output of everyDomainPathsaccessor, so any future drift in the path constants fails in one focused test rather than silently propagating through downstream callers.Confidence Score: 5/5
Pure refactor with no behavior changes; all four call-sites produce the same paths as before, and the new layout test guards against future drift.
All changed paths were verified to be functionally equivalent to the literals they replace —
pkg::TOP_HASHmatches the previously-hardcoded hash in the install test, theOsStrcomparison in the file-system watcher is sound, and theDomainPathstemporary incommands.rshas no lifetime issues. The only gap is that the four private constants insidepaths.rsstill embed a bare".quilt"prefix instead of referencingDOT_QUILT_DIR, but since they are co-located in the same file the risk of unnoticed drift is minimal.No files require special attention; the suggestion in
quilt-rs/src/paths.rsis optional hardening.Important Files Changed
DOT_QUILT_DIRconstant anddot_quilt_dir()accessor onDomainPaths, plus a comprehensive layout-pinning test. The existing private constants still embed the.quiltprefix as bare string literals rather than referencingDOT_QUILT_DIR, but all constants live in the same file so any rename would be visible immediately..quilt/data.jsonpath construction withdomain_paths.lineage(). Theauth_dirpath is intentionally left as a directinfo.data_dir.join(AUTH_DIR)since auth lives outside.quilt; both paths remain correct and consistent.".quilt"string literal in the component filter withquilt::paths::DOT_QUILT_DIR. Functionally identical;OsStr == &strcomparison is sound on all platforms.local_data_dir.join(".quilt")with a temporaryDomainPathsinstance whosedot_quilt_dir()is called immediately; semantically identical, no lifetime issues.DomainPathshelpers. Verified thatpkg::TOP_HASHmatches the previously-hardcoded hash, so the assertions are functionally equivalent.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[DOT_QUILT_DIR const] --> B[dot_quilt_dir method] A --> C[LINEAGE_FILE const] A --> D[INSTALLED_DIR const] A --> E[MANIFEST_DIR const] A --> F[OBJECTS_DIR const] B --> G[commands.rs debug_dot_quilt] A --> H[fswatcher filter is_ignored] C --> I[diagnostics.rs lineage path] D --> J[install.rs installed_manifest] E --> K[install.rs cached_manifests_dir] F --> L[install.rs objects_dir]Comments Outside Diff (1)
quilt-rs/src/paths.rs, line 49-56 (link)DOT_QUILT_DIRstill embed a bare".quilt"prefix instead of referencing the new constant. IfDOT_QUILT_DIRis ever renamed, these four won't follow and every downstream accessor will silently diverge. All constants live in the same file so the risk is low, but usingconcat!makes the dependency explicit and machine-checkable.Reviews (1): Last reviewed commit: "autoformat" | Re-trigger Greptile