feat(cd): support ~ expansion to home directory#15
Conversation
|
Warning Rate limit exceeded@Pawank06 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 8 minutes and 1 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds tilde ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Shell
participant Env as Environment (HOME)
participant FS as Filesystem
User->>Shell: run `cd <target>`
Shell->>Shell: detect if target == "~" or startsWith("~/")
alt tilde path
Shell->>Env: read HOME
Env-->>Shell: HOME value / error
opt HOME success
Shell->>FS: attempt chdir(expanded_path)
FS-->>Shell: success / failure
end
else non-tilde path
Shell->>FS: check exists(target)
FS-->>Shell: exists / missing
opt exists
Shell->>FS: check is_dir(target)
FS-->>Shell: is_dir / not_dir
opt is_dir
Shell->>FS: attempt chdir(target)
FS-->>Shell: success / failure
end
end
end
Shell->>User: print success or specific error message
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
🤖 Fix all issues with AI agents
In @src/main.rs:
- Around line 88-96: The current tilde handling only checks for path == "~" and
should instead detect paths starting with "~" and replace the leading "~" with
the HOME env value before calling env::set_current_dir; update the logic around
the variable path to if path.starts_with('~') { let home = env::var("HOME")?;
let expanded = format!("{}{}", home, &path[1..]);
env::set_current_dir(&expanded) ... } (use env::var("HOME") and
env::set_current_dir accordingly), and fix the error message text from "Couldn't
able to read env variable HOME: {}" to a grammatically correct message such as
"Could not read HOME environment variable: {}" while preserving existing error
handling for env::set_current_dir failures.
🧹 Nitpick comments (2)
src/main.rs (2)
92-92: Inconsistent error message format.Line 92 (and line 104) are missing a colon between the path and error message, unlike lines 99 and 101 which follow the standard Unix format
"cd: <path>: <error>".♻️ Proposed fix for consistent formatting
- eprintln!("cd: {} {}", &val, e) + eprintln!("cd: {}: {}", &val, e)Also apply the same fix to line 104:
- eprintln!("cd: {} {}", path.display(), e); + eprintln!("cd: {}: {}", full_path, e);
98-107: Good validation logic with pre-checks.The pre-validation of path existence and directory status before attempting to change directory is good practice and provides clear user-facing error messages.
Optional: Use
full_pathfor consistency in error message.Line 104 uses
path.display()while lines 99 and 101 usefull_path. For consistency, consider usingfull_pathon line 104 as well.
src/main.rs
Outdated
| if path == "~" { | ||
| match env::var("HOME") { | ||
| Ok(val) => { | ||
| if let Err(e) = env::set_current_dir(&val) { | ||
| eprintln!("cd: {} {}", &val, e) | ||
| } | ||
| } | ||
| Err(e) => eprintln!("Couldn't able to read env variable HOME: {}", e), | ||
| } |
There was a problem hiding this comment.
Incomplete tilde expansion: only handles bare ~, not ~/path.
Standard Unix shells expand ~ at the beginning of any path (e.g., ~/Documents, ~/.config), but this implementation only handles the exact string "~". This deviates from the PR objective to "align cd behavior with standard Unix shells."
Consider expanding any path that starts with ~:
🔧 Proposed fix to support full tilde expansion
- if path == "~" {
+ let path_str = full_path.as_str();
+ if path_str == "~" || path_str.starts_with("~/") {
match env::var("HOME") {
Ok(val) => {
- if let Err(e) = env::set_current_dir(&val) {
+ let expanded = if path_str == "~" {
+ val.clone()
+ } else {
+ path_str.replacen("~", &val, 1)
+ };
+ if let Err(e) = env::set_current_dir(&expanded) {
- eprintln!("cd: {} {}", &val, e)
+ eprintln!("cd: {}: {}", &expanded, e)
}
}
Err(e) => eprintln!("Couldn't able to read env variable HOME: {}", e),
}Fix grammatical error in error message.
Line 95: "Couldn't able to read" is grammatically incorrect.
📝 Proposed fix
- Err(e) => eprintln!("Couldn't able to read env variable HOME: {}", e),
+ Err(e) => eprintln!("cd: Unable to read HOME environment variable: {}", e),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if path == "~" { | |
| match env::var("HOME") { | |
| Ok(val) => { | |
| if let Err(e) = env::set_current_dir(&val) { | |
| eprintln!("cd: {} {}", &val, e) | |
| } | |
| } | |
| Err(e) => eprintln!("Couldn't able to read env variable HOME: {}", e), | |
| } | |
| if path == "~" { | |
| match env::var("HOME") { | |
| Ok(val) => { | |
| if let Err(e) = env::set_current_dir(&val) { | |
| eprintln!("cd: {} {}", &val, e) | |
| } | |
| } | |
| Err(e) => eprintln!("cd: Unable to read HOME environment variable: {}", e), | |
| } |
🤖 Prompt for AI Agents
In @src/main.rs around lines 88 - 96, The current tilde handling only checks for
path == "~" and should instead detect paths starting with "~" and replace the
leading "~" with the HOME env value before calling env::set_current_dir; update
the logic around the variable path to if path.starts_with('~') { let home =
env::var("HOME")?; let expanded = format!("{}{}", home, &path[1..]);
env::set_current_dir(&expanded) ... } (use env::var("HOME") and
env::set_current_dir accordingly), and fix the error message text from "Couldn't
able to read env variable HOME: {}" to a grammatically correct message such as
"Could not read HOME environment variable: {}" while preserving existing error
handling for env::set_current_dir failures.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main.rs (1)
33-43:typeshould includecdas a builtin.
Currentlycdis implemented as a builtin (Line 79+) buttypewon’t report it as one.Proposed fix
- let builtin = ["exit", "echo", "type", "pwd"]; + let builtin = ["exit", "echo", "type", "pwd", "cd"];
🧹 Nitpick comments (1)
src/main.rs (1)
87-112: PreferPathBuf+strip_prefix(andvar_os) overreplacenfor~expansion.
Today this works for"~"/"~/", but string replacement is easy to regress, andenv::var("HOME")fails for non‑UTF8 HOME even thoughset_current_dircan handle it.Proposed refactor (safer expansion + consistent error formatting)
use std::env; use std::fs; use std::io::{self, Write}; use std::os::unix::fs::PermissionsExt; use std::os::unix::process::CommandExt; use std::path::Path; +use std::path::PathBuf; use std::process::Command; @@ let args = &parts[1..]; let full_path = args.join(" "); let path = Path::new(&full_path); let path_str = full_path.as_str(); if path_str == "~" || path_str.starts_with("~/") { - match env::var("HOME") { - Ok(val) => { - let expanded = if path_str == "~" { - val.clone() - } else { - path_str.replacen("~", &val, 1) - }; - - if let Err(e) = env::set_current_dir(&expanded) { - eprintln!("cd: {} {}", &expanded, e) - } - } - Err(e) => eprintln!("cd: Unable to read HOME environment variable: {}", e), - } + match env::var_os("HOME") { + Some(home) if !home.is_empty() => { + let expanded: PathBuf = if path_str == "~" { + home.into() + } else { + let suffix = path_str.strip_prefix("~/").unwrap(); + PathBuf::from(home).join(suffix) + }; + + if let Err(e) = env::set_current_dir(&expanded) { + eprintln!("cd: {}: {}", expanded.display(), e); + } + } + _ => eprintln!("cd: HOME is not set"), + }; } else { if !path.exists() { - println!("cd: {}: No such file or directory", full_path); + eprintln!("cd: {}: No such file or directory", full_path); } else if !path.is_dir() { - println!("cd: {}: Not a directory", full_path); + eprintln!("cd: {}: Not a directory", full_path); } else { if let Err(e) = env::set_current_dir(&full_path) { - eprintln!("cd: {} {}", path.display(), e); + eprintln!("cd: {}: {}", path.display(), e); } } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main.rs
🔇 Additional comments (1)
src/main.rs (1)
44-72: Good: PATH read failure is now sent to stderr with clearer wording.
Line 71: switching toeprintln!is the right direction for error output.
1 participant
summary
notes