Skip to content

Commit a11ab3c

Browse files
overtrueovertrue
authored
[codex] fix local path alias ambiguity in cp and mv (#27)
* feat(phase-2): fix local path alias ambiguity in cp and mv * feat(phase-2): address PR feedback on cp/mv path disambiguation
1 parent 5859b8c commit a11ab3c

2 files changed

Lines changed: 138 additions & 5 deletions

File tree

‎crates/cli/src/commands/cp.rs‎

Lines changed: 69 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ use clap::Args;
66
use rc_core::{AliasManager, ObjectStore as _, ParsedPath, RemotePath, parse_path};
77
use rc_s3::S3Client;
88
use serde::Serialize;
9-
use std::path::Path;
9+
use std::path::{Path, PathBuf};
1010

1111
use crate::exit_code::ExitCode;
1212
use crate::output::{Formatter, OutputConfig};
@@ -63,17 +63,18 @@ struct CpOutput {
6363
/// Execute the cp command
6464
pub async fn execute(args: CpArgs, output_config: OutputConfig) -> ExitCode {
6565
let formatter = Formatter::new(output_config);
66+
let alias_manager = AliasManager::new().ok();
6667

6768
// Parse source and target paths
68-
let source = match parse_path(&args.source) {
69+
let source = match parse_cp_path(&args.source, alias_manager.as_ref()) {
6970
Ok(p) => p,
7071
Err(e) => {
7172
formatter.error(&format!("Invalid source path: {e}"));
7273
return ExitCode::UsageError;
7374
}
7475
};
7576

76-
let target = match parse_path(&args.target) {
77+
let target = match parse_cp_path(&args.target, alias_manager.as_ref()) {
7778
Ok(p) => p,
7879
Err(e) => {
7980
formatter.error(&format!("Invalid target path: {e}"));
@@ -102,6 +103,26 @@ pub async fn execute(args: CpArgs, output_config: OutputConfig) -> ExitCode {
102103
}
103104
}
104105

106+
fn parse_cp_path(path: &str, alias_manager: Option<&AliasManager>) -> rc_core::Result<ParsedPath> {
107+
let parsed = parse_path(path)?;
108+
109+
let ParsedPath::Remote(remote) = &parsed else {
110+
return Ok(parsed);
111+
};
112+
113+
if let Some(manager) = alias_manager
114+
&& matches!(manager.exists(&remote.alias), Ok(true))
115+
{
116+
return Ok(parsed);
117+
}
118+
119+
if Path::new(path).exists() {
120+
return Ok(ParsedPath::Local(PathBuf::from(path)));
121+
}
122+
123+
Ok(parsed)
124+
}
125+
105126
async fn copy_local_to_s3(
106127
src: &Path,
107128
dst: &RemotePath,
@@ -583,6 +604,16 @@ async fn copy_s3_to_s3(
583604
#[cfg(test)]
584605
mod tests {
585606
use super::*;
607+
use rc_core::{Alias, ConfigManager};
608+
use tempfile::TempDir;
609+
610+
fn temp_alias_manager() -> (AliasManager, TempDir) {
611+
let temp_dir = TempDir::new().expect("create temp dir");
612+
let config_path = temp_dir.path().join("config.toml");
613+
let config_manager = ConfigManager::with_path(config_path);
614+
let alias_manager = AliasManager::with_config_manager(config_manager);
615+
(alias_manager, temp_dir)
616+
}
586617

587618
#[test]
588619
fn test_parse_local_path() {
@@ -639,6 +670,41 @@ mod tests {
639670
}
640671
}
641672

673+
#[test]
674+
fn test_parse_cp_path_prefers_existing_local_path_when_alias_missing() {
675+
let (alias_manager, temp_dir) = temp_alias_manager();
676+
let full = temp_dir.path().join("issue-2094-local").join("file.txt");
677+
let full_str = full.to_string_lossy().to_string();
678+
679+
if let Some(parent) = full.parent() {
680+
std::fs::create_dir_all(parent).expect("create parent dirs");
681+
}
682+
std::fs::write(&full, b"test").expect("write local file");
683+
684+
let parsed = parse_cp_path(&full_str, Some(&alias_manager)).expect("parse path");
685+
assert!(matches!(parsed, ParsedPath::Local(_)));
686+
}
687+
688+
#[test]
689+
fn test_parse_cp_path_keeps_remote_when_alias_exists() {
690+
let (alias_manager, _temp_dir) = temp_alias_manager();
691+
alias_manager
692+
.set(Alias::new("target", "http://localhost:9000", "a", "b"))
693+
.expect("set alias");
694+
695+
let parsed = parse_cp_path("target/bucket/file.txt", Some(&alias_manager))
696+
.expect("parse remote path");
697+
assert!(matches!(parsed, ParsedPath::Remote(_)));
698+
}
699+
700+
#[test]
701+
fn test_parse_cp_path_keeps_remote_when_local_missing() {
702+
let (alias_manager, _temp_dir) = temp_alias_manager();
703+
let parsed = parse_cp_path("missing/bucket/file.txt", Some(&alias_manager))
704+
.expect("parse remote path");
705+
assert!(matches!(parsed, ParsedPath::Remote(_)));
706+
}
707+
642708
#[test]
643709
fn test_cp_args_defaults() {
644710
let args = CpArgs {

‎crates/cli/src/commands/mv.rs‎

Lines changed: 69 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ use clap::Args;
66
use rc_core::{AliasManager, ObjectStore as _, ParsedPath, RemotePath, parse_path};
77
use rc_s3::S3Client;
88
use serde::Serialize;
9+
use std::path::{Path, PathBuf};
910

1011
use crate::exit_code::ExitCode;
1112
use crate::output::{Formatter, OutputConfig};
@@ -44,17 +45,18 @@ struct MvOutput {
4445
/// Execute the mv command
4546
pub async fn execute(args: MvArgs, output_config: OutputConfig) -> ExitCode {
4647
let formatter = Formatter::new(output_config);
48+
let alias_manager = AliasManager::new().ok();
4749

4850
// Parse source and target paths
49-
let source = match parse_path(&args.source) {
51+
let source = match parse_mv_path(&args.source, alias_manager.as_ref()) {
5052
Ok(p) => p,
5153
Err(e) => {
5254
formatter.error(&format!("Invalid source path: {e}"));
5355
return ExitCode::UsageError;
5456
}
5557
};
5658

57-
let target = match parse_path(&args.target) {
59+
let target = match parse_mv_path(&args.target, alias_manager.as_ref()) {
5860
Ok(p) => p,
5961
Err(e) => {
6062
formatter.error(&format!("Invalid target path: {e}"));
@@ -83,6 +85,26 @@ pub async fn execute(args: MvArgs, output_config: OutputConfig) -> ExitCode {
8385
}
8486
}
8587

88+
fn parse_mv_path(path: &str, alias_manager: Option<&AliasManager>) -> rc_core::Result<ParsedPath> {
89+
let parsed = parse_path(path)?;
90+
91+
let ParsedPath::Remote(remote) = &parsed else {
92+
return Ok(parsed);
93+
};
94+
95+
if let Some(manager) = alias_manager
96+
&& matches!(manager.exists(&remote.alias), Ok(true))
97+
{
98+
return Ok(parsed);
99+
}
100+
101+
if Path::new(path).exists() {
102+
return Ok(ParsedPath::Local(PathBuf::from(path)));
103+
}
104+
105+
Ok(parsed)
106+
}
107+
86108
async fn move_local_to_s3(
87109
src: &std::path::Path,
88110
dst: &RemotePath,
@@ -292,6 +314,16 @@ async fn move_s3_to_s3(
292314
#[cfg(test)]
293315
mod tests {
294316
use super::*;
317+
use rc_core::{Alias, ConfigManager};
318+
use tempfile::TempDir;
319+
320+
fn temp_alias_manager() -> (AliasManager, TempDir) {
321+
let temp_dir = TempDir::new().expect("create temp dir");
322+
let config_path = temp_dir.path().join("config.toml");
323+
let config_manager = ConfigManager::with_path(config_path);
324+
let alias_manager = AliasManager::with_config_manager(config_manager);
325+
(alias_manager, temp_dir)
326+
}
295327

296328
#[test]
297329
fn test_parse_paths() {
@@ -331,6 +363,41 @@ mod tests {
331363
}
332364
}
333365

366+
#[test]
367+
fn test_parse_mv_path_prefers_existing_local_path_when_alias_missing() {
368+
let (alias_manager, temp_dir) = temp_alias_manager();
369+
let full = temp_dir.path().join("issue-2094-mv-local").join("file.txt");
370+
let full_str = full.to_string_lossy().to_string();
371+
372+
if let Some(parent) = full.parent() {
373+
std::fs::create_dir_all(parent).expect("create parent dirs");
374+
}
375+
std::fs::write(&full, b"test").expect("write local file");
376+
377+
let parsed = parse_mv_path(&full_str, Some(&alias_manager)).expect("parse path");
378+
assert!(matches!(parsed, ParsedPath::Local(_)));
379+
}
380+
381+
#[test]
382+
fn test_parse_mv_path_keeps_remote_when_alias_exists() {
383+
let (alias_manager, _temp_dir) = temp_alias_manager();
384+
alias_manager
385+
.set(Alias::new("target", "http://localhost:9000", "a", "b"))
386+
.expect("set alias");
387+
388+
let parsed = parse_mv_path("target/bucket/file.txt", Some(&alias_manager))
389+
.expect("parse remote path");
390+
assert!(matches!(parsed, ParsedPath::Remote(_)));
391+
}
392+
393+
#[test]
394+
fn test_parse_mv_path_keeps_remote_when_local_missing() {
395+
let (alias_manager, _temp_dir) = temp_alias_manager();
396+
let parsed = parse_mv_path("missing/bucket/file.txt", Some(&alias_manager))
397+
.expect("parse remote path");
398+
assert!(matches!(parsed, ParsedPath::Remote(_)));
399+
}
400+
334401
#[test]
335402
fn test_mv_args_defaults() {
336403
let args = MvArgs {

0 commit comments

Comments
 (0)