fix: make cp -r with a trailing /. copy directory contents like GNU cp#185
Conversation
There was a problem hiding this comment.
DIsclosure: All AI generated, but i reviewed it.
bartlomieju
left a comment
There was a problem hiding this comment.
Thanks. It looks mostly good, but I did second pass with an agent and got this:
cp doesn't get the same ./.. guard that mv does. The mv path bails on ./.., but the shared cp path still resolves .. through calculate_destination_path:
b".." => destination.join(".."),So cp -r foo/.. dist (with dist an existing dir) computes to_path = dist/.. → cwd, and from_path = foo/.. → cwd. do_copy_operation then runs copy_dir_recursively(cwd, cwd). Since std::fs::copy opens the destination with truncate, same-path copies zero the files out, so this is potential data loss across the whole directory.
Before this PR the same command panicked on file_name().unwrap() (aborted, no data touched), so this is a behavior change worth closing off. Could we give cp the same guard as mv (bail on a trailing ./..), rather than resolving .. to destination.join("..")?. A test along the lines of the mv one would lock it in.
|
Updated. |
Fixes #176.
cp -r public/. dist/createddist/publicinstead of merging the contents ofpublicintodist. The destination was computed withPath::file_name(), which normalizes a trailing.component away (Path::new("public/.").file_name()isSome("public")). GNU cp uses the final component of the path as written — forpublic/.that's., so the target resolves todist/.=distitself.Changes:
.merges directory contents into the destination (hidden files included)file_name()returnsNonefor a source ending in..and it was unwrappedmv, added a guard somv public/. disterrors withrefusing to move '.' or '..'like GNU mv instead of potentially renaming over the destination directory