API-98 // Change the versions dir location in config.toml to relative path #223
Conversation
WalkthroughThe PR changes the Changes
Sequence Diagram(s)(omitted — changes are API/instantiation and path-resolution updates without new multi-component control flows requiring visualization) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🧹 Nitpick comments (1)
oxen-rust/src/lib/src/storage/version_store.rs (1)
218-224: Consider using a more robust relative path check.The current implementation checks if
path.starts_with(".oxen")to determine whether to resolve the path relative torepo_dir. However,PathBuf::starts_withchecks path components, which means it would match paths likePathBuf::from(".oxen/versions/files")but not paths that have been canonicalized or normalized differently.Consider using
path.is_relative()for a more general and robust check, which would correctly handle any relative path regardless of whether it starts with ".oxen":- let versions_dir = if let Some(path) = &local_storage_opts.path { - if path.starts_with(".oxen") { - // if the path is relative, convert to absolute path - repo_dir.join(path) - } else { - path.clone() - } + let versions_dir = if let Some(ref path) = local_storage_opts.path { + if path.is_relative() { + // if the path is relative, convert to absolute path + repo_dir.join(path) + } else { + path.clone() + } } else {This approach would correctly handle all relative paths (e.g., ".oxen/versions/files", "./data", "versions", etc.) while treating absolute paths uniformly.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
oxen-rust/src/lib/src/core/v_latest/prune.rs(1 hunks)oxen-rust/src/lib/src/model/repository/local_repository.rs(5 hunks)oxen-rust/src/lib/src/storage/version_store.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-18T19:11:42.443Z
Learnt from: Lilianxr
Repo: Oxen-AI/Oxen PR: 146
File: oxen-rust/src/benches/oxen/download.rs:176-179
Timestamp: 2025-09-18T19:11:42.443Z
Learning: In oxen-rust/src/benches/oxen/download.rs, the setup_repo_for_download_benchmark function always stages files under a "files" directory structure in the repository, regardless of whether data_path is provided or not. The hardcoded Path::new("files") in the repositories::download call is correct because of this consistent setup.
Applied to files:
oxen-rust/src/lib/src/storage/version_store.rsoxen-rust/src/lib/src/model/repository/local_repository.rs
🧬 Code graph analysis (3)
oxen-rust/src/lib/src/core/v_latest/prune.rs (2)
oxen-rust/src/lib/src/model/repository/local_repository.rs (1)
version_store(83-88)oxen-rust/src/lib/src/storage/version_store.rs (1)
create_version_store(208-251)
oxen-rust/src/lib/src/storage/version_store.rs (3)
oxen-rust/src/lib/src/util/fs.rs (1)
oxen_hidden_dir(46-48)oxen-rust/src/lib/src/model/repository/local_repository.rs (1)
new(128-155)oxen-rust/src/lib/src/storage/local.rs (1)
new(31-35)
oxen-rust/src/lib/src/model/repository/local_repository.rs (2)
oxen-rust/src/lib/src/storage/version_store.rs (1)
create_version_store(208-251)oxen-rust/src/lib/src/util/fs.rs (3)
is_relative_to_dir(1496-1555)oxen_hidden_dir(46-48)path_relative_to_dir(1384-1493)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test Suite / Test Suite (windows-latest)
- GitHub Check: Test Suite / Test Suite (ubuntu-latest)
- GitHub Check: Test Suite / Test Suite (macos-latest)
- GitHub Check: Lint / Cargo check, format, clippy + Ruff
🔇 Additional comments (4)
oxen-rust/src/lib/src/core/v_latest/prune.rs (1)
284-284: LGTM! Signature update correctly applied.The call to
create_version_storehas been updated to pass&repo.pathas the first argument, matching the new function signature.oxen-rust/src/lib/src/model/repository/local_repository.rs (2)
14-14: LGTM! HashMap import added for StorageConfig construction.The HashMap import is necessary for constructing the settings field in the
savemethod.
77-77: LGTM! All create_version_store calls updated consistently.All call sites have been correctly updated to pass the repository path as the first argument, matching the new function signature.
Also applies to: 91-91, 102-102, 109-109
oxen-rust/src/lib/src/storage/version_store.rs (1)
209-209: LGTM! Function signature updated to enable relative path resolution.The addition of the
repo_dirparameter enables proper resolution of relative storage paths, which is the core objective of this PR.
rpschoenburg
left a comment
rpschoenburg
left a comment
There was a problem hiding this comment.
Looks good! I do agree with the coderabbit comment to remove the unwrap statement though, that should be cleaned up
To test:
Run
oxen initand check the .oxen/config.toml to find the path in storage.settings is ".oxen/versions/files"Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.