-
Notifications
You must be signed in to change notification settings - Fork 72
feat: 新增 open 指令并增加相应的单元测试和集成测试 #169
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -104,6 +104,8 @@ enum Commands { | |
| Revert(command::revert::RevertArgs), | ||
| #[command(subcommand, about = "Manage set of tracked repositories")] | ||
| Remote(command::remote::RemoteCmds), | ||
| #[command(about = "Open the repository in the browser")] | ||
| Open(command::open::OpenArgs), | ||
| #[command(about = "Manage repository configurations")] | ||
| Config(command::config::ConfigArgs), | ||
| #[command(about = "Manage the log of reference changes (e.g., HEAD, branches)")] | ||
|
|
@@ -209,6 +211,7 @@ pub async fn parse_async(args: Option<&[&str]>) -> Result<(), GitError> { | |
| Commands::Blame(args) => command::blame::execute(args).await, | ||
| Commands::Revert(args) => command::revert::execute(args).await, | ||
| Commands::Remote(cmd) => command::remote::execute(cmd).await, | ||
| Commands::Open(args) => command::open::open(args).await, | ||
|
||
| Commands::Pull(args) => command::pull::execute(args).await, | ||
| Commands::Config(args) => command::config::execute(args).await, | ||
| Commands::Checkout(args) => command::checkout::execute(args).await, | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,114 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||
| use std::process::Command; | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| use clap::Parser; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| use regex::Regex; | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| use crate::internal::config::Config; | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| #[derive(Parser, Debug)] | ||||||||||||||||||||||||||||||||||||||||||||||||||
| pub struct OpenArgs { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| /// The remote to open | ||||||||||||||||||||||||||||||||||||||||||||||||||
| pub remote: Option<String>, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| pub async fn open(args: OpenArgs) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| let remote_url = if let Some(remote_name) = args.remote { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| let remotes = Config::all_remote_configs().await; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if remotes.iter().any(|r| r.name == remote_name) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| Config::get_remote_url(&remote_name).await | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| if remotes.iter().any(|r| r.name == remote_name) { | |
| Config::get_remote_url(&remote_name).await | |
| if let Some(remote) = remotes.iter().find(|r| r.name == remote_name) { | |
| if remote.url.is_empty() { | |
| eprintln!("fatal: Remote '{}' has no URL configured", remote_name); | |
| return; | |
| } | |
| remote.url.clone() |
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.
Avoid panic on detached HEAD in open default path
libra open calls Config::get_current_remote_url() here when no remote is supplied; that helper unwrap()s and panics if HEAD is detached. In that scenario, the command crashes before reaching the fallback to origin/first remote, so running open in a detached HEAD state will unexpectedly abort. Consider handling the detached-HEAD error and continuing to the fallback branch instead of unwrapping.
Useful? React with 👍 / 👎.
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.
Guard against detached HEAD before resolving default remote
When no remote is provided, this path calls Config::get_current_remote_url().await directly. That helper unwraps the result of get_current_remote_with_conn (see src/internal/config.rs around the unwrap() at lines 152-154), which panics on detached HEAD. Running libra open in a detached HEAD repo will therefore crash instead of emitting a user-friendly error. Consider checking get_current_remote_with_conn for Err (or providing a safe wrapper) before calling here so the command can report the detached HEAD condition gracefully.
Useful? React with 👍 / 👎.
Copilot
AI
Jan 27, 2026
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.
The fallback logic for finding a remote has potential issues. When Config::get_current_remote_url() returns None, the code falls back to "origin", then to the first remote. However, this behavior is not clearly communicated to the user. Consider adding informational messages like "Using remote 'origin'" or "Using remote 'X' (first available)" so users understand which remote is being opened. This is especially important when multiple remotes exist.
Outdated
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.
Security concern: Command injection vulnerability
The URL is passed directly to the system shell without proper escaping. A malicious remote URL could potentially execute arbitrary commands.
Example attack vector:
remote.origin.url = "https://example.com; rm -rf /"
Recommendation:
Either:
- Validate and sanitize the URL before passing it to the shell command
- Use proper argument escaping/quoting
- Validate URL format with regex before execution
Example fix:
// Validate URL format
if !url.starts_with("http://") && !url.starts_with("https://") {
eprintln!("fatal: Invalid URL format");
return;
}
// On Windows, use proper argument escaping
Command::new("cmd")
.args(["/C", "start", ""]) // Empty string forces URL to be treated as argument
.arg(&url)
.spawn()
.ok();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.
@Tyooughtul , 对于注入的安全问题,请参考 Claude 的 Review 意见进行修复
Outdated
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.
Security concern: Command injection vulnerability (macOS)
Same issue as Windows - the URL needs proper escaping. On macOS, open interprets certain characters specially.
Recommendation:
Command::new("open")
.arg("--") // Ensures URL is treated as argument, not option
.arg(&url)
.spawn()
.ok();
Outdated
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.
Security concern: Command injection vulnerability (Linux)
Same issue - xdg-open can be exploited with malicious URLs.
Recommendation:
Command::new("xdg-open")
.arg(&url)
.spawn()
.ok();Note: Using .arg() separately (not in the command string) provides better protection than shell interpolation.
Outdated
Copilot
AI
Jan 27, 2026
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.
The Command::spawn() calls have their errors silently discarded with .ok(). If the browser fails to open (e.g., command not found, permission denied), the user will see "Opening URL" but the browser won't actually open and they'll get no indication of what went wrong. Consider handling the error and providing user feedback. For example, you could check the Result and print a warning if the spawn fails.
| Command::new("cmd").args(["/C", "start", &url]).spawn().ok(); | |
| } | |
| #[cfg(target_os = "macos")] | |
| { | |
| Command::new("open").arg(&url).spawn().ok(); | |
| } | |
| #[cfg(all(not(target_os = "windows"), not(target_os = "macos")))] | |
| { | |
| Command::new("xdg-open").arg(&url).spawn().ok(); | |
| if let Err(err) = Command::new("cmd").args(["/C", "start", &url]).spawn() { | |
| eprintln!("warning: failed to open URL '{}' in browser: {}", url, err); | |
| } | |
| } | |
| #[cfg(target_os = "macos")] | |
| { | |
| if let Err(err) = Command::new("open").arg(&url).spawn() { | |
| eprintln!("warning: failed to open URL '{}' in browser: {}", url, err); | |
| } | |
| } | |
| #[cfg(all(not(target_os = "windows"), not(target_os = "macos")))] | |
| { | |
| if let Err(err) = Command::new("xdg-open").arg(&url).spawn() { | |
| eprintln!("warning: failed to open URL '{}' in browser: {}", url, err); | |
| } |
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.
Error handling: Silent failures
All three platform-specific commands use .ok() which silently ignores errors. Users won't know if:
- The command doesn't exist on their system
- The URL format is invalid
- The browser launch failed
Recommendation:
let result = Command::new("open").arg(&url).spawn();
match result {
Ok(_) => {},
Err(e) => eprintln!("warning: Failed to open browser: {}", e),
}
Outdated
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.
Potential panic: unwrap() on regex
While this regex is valid and won't panic in practice, using unwrap() on Regex::new() is considered a code smell.
Best practice:
Use lazy_static or once_cell to compile the regex once at startup:
use once_cell::sync::Lazy;
static SCP_RE: Lazy<Regex> = Lazy::new(|| {
Regex::new(r"^git@([^:]+):(.+?)(\.git)?$").expect("Invalid SCP regex")
});
// Then use: if let Some(caps) = SCP_RE.captures(remote) { ... }This is more efficient and makes the panic cause clearer if it happens during initialization.
Outdated
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.
Parse SCP-style SSH URLs with non-git user
The SCP-like parser only matches git@host:path (^git@...) so remotes like alice@github.com:org/repo.git (or enterprise setups with a different SSH user) won’t be transformed into an HTTPS URL. Those inputs fall through to xdg-open/open as a raw SSH string, which typically can’t be opened in a browser, so libra open silently fails for a common remote format. The regex should accept any user@ prefix (or parse generically) to cover these cases.
Useful? React with 👍 / 👎.
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.
Code clarity: Direct string formatting
Minor: The format! macro can be simplified when concatenating with /:
return format!("https://{host}/{path}");This is more idiomatic Rust.
Copilot
AI
Jan 27, 2026
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.
The SCP-style regex pattern (line 64) assumes "git@" as the user, but SCP syntax can use any username. For example, "john@gitlab.com:repo/project.git" is valid Git syntax but won't match this pattern. Consider making the user part generic by changing the regex to match any user, e.g., r"^([^@]+)@([^:]+):(.+?)(\.git)?$" and capturing both user and host, or at minimum updating the regex to handle any username.
Outdated
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.
Same issue: unwrap() on regex
Same recommendation as above - use lazy_static or once_cell for the SSH regex.
Outdated
Copilot
AI
Jan 27, 2026
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.
The Regex compilation happens on every call to transform_url. According to Rust performance best practices and the custom guidelines emphasizing performance optimization, these regexes should be compiled once using lazy_static or std::sync::OnceLock to avoid repeated compilation overhead. This is especially important as this function is in a hot path called for every URL transformation.
Outdated
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.
Same as above: Can simplify to format!("https://{host}/{path}")
Outdated
Copilot
AI
Jan 27, 2026
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.
Typo in the comment. Should be "Unit test" instead of "Uint test".
| // Uint test | |
| // Unit test |
Outdated
Copilot
AI
Jan 27, 2026
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.
Typo in comment: "Uint test" should be "Unit test".
| // Uint test | |
| // Unit test |
Copilot
AI
Jan 27, 2026
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.
The unit tests don't cover several important edge cases:
- URLs with usernames other than 'git' in SCP format (e.g., 'myuser@github.com:repo/path.git')
- HTTPS URLs with trailing slashes
- URLs without .git suffix
- Malformed URLs that should fail gracefully
- File:// URLs (should be rejected by is_safe_url)
- URLs with query parameters or fragments
Consider adding test cases for these scenarios to ensure robust URL handling.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| //! Tests open command integration to ensure it finds remote correctly. | ||
| use libra::{ | ||
| command::{ | ||
| open, | ||
| remote::{self, RemoteCmds}, | ||
| }, | ||
| utils::test, | ||
| }; | ||
| use serial_test::serial; | ||
| use tempfile::tempdir; | ||
|
|
||
| #[tokio::test] | ||
| #[serial] | ||
| async fn test_open_remote_origin() { | ||
| let repo_dir = tempdir().unwrap(); | ||
| test::setup_with_new_libra_in(repo_dir.path()).await; | ||
| let _guard = test::ChangeDirGuard::new(repo_dir.path()); | ||
|
|
||
| // Add origin remote | ||
| remote::execute(RemoteCmds::Add { | ||
| name: "origin".into(), | ||
| url: "git@github.com:user/repo.git".into(), | ||
| }) | ||
| .await; | ||
|
|
||
| // Test explicit remote | ||
| open::open(open::OpenArgs { | ||
| remote: Some("origin".to_string()), | ||
| }) | ||
| .await; | ||
|
|
||
| // Test default remote should find origin | ||
| open::open(open::OpenArgs { remote: None }).await; | ||
|
||
| } | ||
|
Comment on lines
12
to
51
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test coverage: Limited assertions The test doesn't verify:
Recommendation: #[tokio::test]
#[serial]
async fn test_open_nonexistent_remote() {
let repo_dir = tempdir().unwrap();
test::setup_with_new_libra_in(repo_dir.path()).await;
let _guard = test::ChangeDirGuard::new(repo_dir.path());
// Should print error message and not panic
open::open(open::OpenArgs {
remote: Some("nonexistent".to_string()),
})
.await;
}
#[tokio::test]
#[serial]
async fn test_open_no_remotes_configured() {
let repo_dir = tempdir().unwrap();
test::setup_with_new_libra_in(repo_dir.path()).await;
let _guard = test::ChangeDirGuard::new(repo_dir.path());
// Should print error message and not panic
open::open(open::OpenArgs { remote: None }).await;
}
Comment on lines
44
to
51
|
||
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.
The function call should be 'command::open::execute(args)' instead of 'command::open::open(args)' to match the naming convention established in this file. All other commands use the execute function (see lines 209-213: blame::execute, revert::execute, remote::execute, pull::execute, config::execute, etc.).