xxchan commented on code in PR #6140:
URL: https://github.com/apache/opendal/pull/6140#discussion_r2083900794
##########
bin/oli/src/commands/cp.rs:
##########
@@ -84,28 +124,67 @@ impl CopyCmd {
return Ok(());
}
- let dst_root = Path::new(&dst_path);
+ // Recursive copy: Ensure the base destination directory exists or
create it.
+ // Note: final_dst_path here refers to the original dst_path if it was
a dir or didn't exist.
+ match dst_op.stat(&final_dst_path).await {
+ Ok(meta) if meta.mode().is_dir() => {
+ // Base destination directory exists.
+ }
+ Ok(_) => {
+ bail!(
+ "Recursive copy destination '{}' exists but is not a
directory.",
+ final_dst_path
+ );
+ }
+ Err(e) if e.kind() == ErrorKind::NotFound => {
+ dst_op.create_dir(&final_dst_path).await?;
+ }
+ Err(e) => {
+ // Another error occurred trying to stat the base destination.
+ return Err(e.into());
+ }
+ }
+
+ // Proceed with recursive copy logic. dst_root is the target directory.
+ let dst_root = Path::new(&final_dst_path);
let mut ds = src_op.lister_with(&src_path).recursive(true).await?;
- let prefix = src_path.strip_prefix('/').unwrap_or(src_path.as_str());
+ let prefix = src_path.strip_prefix('/').unwrap_or(&src_path); // Using
original logic for now
Review Comment:
What does the comment mean here :eyes:
##########
bin/oli/src/commands/cp.rs:
##########
@@ -84,28 +124,67 @@ impl CopyCmd {
return Ok(());
}
- let dst_root = Path::new(&dst_path);
+ // Recursive copy: Ensure the base destination directory exists or
create it.
+ // Note: final_dst_path here refers to the original dst_path if it was
a dir or didn't exist.
+ match dst_op.stat(&final_dst_path).await {
+ Ok(meta) if meta.mode().is_dir() => {
+ // Base destination directory exists.
+ }
+ Ok(_) => {
+ bail!(
+ "Recursive copy destination '{}' exists but is not a
directory.",
+ final_dst_path
+ );
+ }
+ Err(e) if e.kind() == ErrorKind::NotFound => {
+ dst_op.create_dir(&final_dst_path).await?;
+ }
+ Err(e) => {
+ // Another error occurred trying to stat the base destination.
+ return Err(e.into());
+ }
+ }
+
+ // Proceed with recursive copy logic. dst_root is the target directory.
+ let dst_root = Path::new(&final_dst_path);
let mut ds = src_op.lister_with(&src_path).recursive(true).await?;
- let prefix = src_path.strip_prefix('/').unwrap_or(src_path.as_str());
+ let prefix = src_path.strip_prefix('/').unwrap_or(&src_path); // Using
original logic for now
+
while let Some(de) = ds.try_next().await? {
let meta = de.metadata();
+ let depath = de.path();
+
+ // Calculate relative path from the source root
+ let relative_path = depath.strip_prefix(prefix).ok_or_else(|| {
+ anyhow::anyhow!(
+ "Internal error: Failed to strip prefix '{}' from path
'{}'",
+ prefix,
+ depath
+ )
+ })?;
Review Comment:
nit: use `.with_context()` instead of `ok_or_else`?
##########
bin/oli/tests/cp.rs:
##########
@@ -61,3 +61,40 @@ async fn test_cp_for_path_in_current_dir() -> Result<()> {
assert_eq!(expect, actual);
Ok(())
}
+
+#[tokio::test]
+async fn test_cp_file_to_existing_dir() -> Result<()> {
Review Comment:
Also add a test for recursive copy?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@opendal.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
