-
Notifications
You must be signed in to change notification settings - Fork 19
Fix panic in oxen diff outside repo #200
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughWhen repository initialization fails, the diff operation now falls back to direct file-path comparison instead of only erroring. The change captures path arguments, validates path_2 presence, calls diff_files with both paths, and wraps the result in a Vec. Existing repo-based behavior is preserved. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
oxen-rust/src/lib/src/repositories/diffs.rs (1)
84-104: Repo init failure fallback looks good; consider guarding against revision-based invocationsThe new handling on
LocalRepository::from_*error is solid: the panic/unwrap onpath_2is gone, you return a clearOxenErrorwhenpath_2is missing, and otherwise cleanly fall back todiff_files(path_1, path_2, …)and wrap the result in aVec. The early return also means movingopts.path_1here is safe and doesn’t affect the latermatchon(opts.path_2, opts.revision_1, opts.revision_2).One subtle behavior change to think about: if
revision_1orrevision_2are set but opening the repo fails, this branch will still run the plain file diff and silently ignore the revision options. That might surprise users who thought they were diffing specific revisions. You might want to:
- Only take the direct file-path fallback when
revision_1.is_none()andrevision_2.is_none(), and otherwise return an error explaining that revision-based diffs require a valid repo, or- Include the presence of revisions in the error message instead of attempting
diff_files.Functionally this is already a big improvement over panic, so this is mostly about tightening semantics and UX around revision arguments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
oxen-rust/src/lib/src/repositories/diffs.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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/repositories/diffs.rs
🧬 Code graph analysis (1)
oxen-rust/src/lib/src/repositories/diffs.rs (1)
oxen-rust/src/lib/src/error.rs (1)
basic_str(158-160)
⏰ 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: Lint / Cargo check, format, clippy + Ruff
- GitHub Check: Test Suite / Test Suite (ubuntu-latest)
- GitHub Check: Test Suite / Test Suite (macos-latest)
Changes a panic to an error.
No important or functional difference in the code
Summary by CodeRabbit
Bug Fixes
New Features
✏️ Tip: You can customize this high-level summary in your review settings.