Skip to content
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

cp: --preserve should keep xattr #5834

Merged
merged 4 commits into from
Jan 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -500,6 +500,7 @@ rlimit = "0.10.1"
[target.'cfg(unix)'.dev-dependencies]
nix = { workspace = true, features = ["process", "signal", "user"] }
rand_pcg = "0.3"
xattr = { workspace = true }

[build-dependencies]
phf_codegen = { workspace = true }
Expand Down
5 changes: 3 additions & 2 deletions src/uu/cp/src/cp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -825,6 +825,7 @@ impl Attributes {
ownership: Preserve::Yes { required: true },
mode: Preserve::Yes { required: true },
timestamps: Preserve::Yes { required: true },
xattr: Preserve::Yes { required: true },
..Self::NONE
};

Expand Down Expand Up @@ -1440,7 +1441,7 @@ pub(crate) fn copy_attributes(
})?;

handle_preserve(&attributes.xattr, || -> CopyResult<()> {
#[cfg(unix)]
#[cfg(all(unix, not(target_os = "android")))]
{
let xattrs = xattr::list(source)?;
for attr in xattrs {
Expand All @@ -1449,7 +1450,7 @@ pub(crate) fn copy_attributes(
}
}
}
#[cfg(not(unix))]
#[cfg(not(all(unix, not(target_os = "android"))))]
{
// The documentation for GNU cp states:
//
Expand Down
58 changes: 57 additions & 1 deletion tests/by-util/test_cp.rs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it the expected behavior that the test doesn't fail if the changes in cp.rs are not applied?

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, i am planning to fix mac and the tests once they land :)

Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
//
// For the full copyright and license information, please view the LICENSE
// file that was distributed with this source code.
// spell-checker:ignore (flags) reflink (fs) tmpfs (linux) rlimit Rlim NOFILE clob btrfs ROOTDIR USERDIR procfs outfile uufs
// spell-checker:ignore (flags) reflink (fs) tmpfs (linux) rlimit Rlim NOFILE clob btrfs ROOTDIR USERDIR procfs outfile uufs xattrs

use crate::common::util::TestScenario;
#[cfg(not(windows))]
Expand Down Expand Up @@ -56,6 +56,8 @@
static TEST_MOUNT_OTHER_FILESYSTEM_FILE: &str = "mount/DO_NOT_copy_me.txt";
#[cfg(unix)]
static TEST_NONEXISTENT_FILE: &str = "nonexistent_file.txt";
#[cfg(all(unix, not(any(target_os = "android", target_os = "macos"))))]
use xattr;

/// Assert that mode, ownership, and permissions of two metadata objects match.
#[cfg(all(not(windows), not(target_os = "freebsd")))]
Expand Down Expand Up @@ -3701,3 +3703,57 @@
.fails()
.stderr_is("cp: 'no-such/' is not a directory\n");
}

#[cfg(all(unix, not(any(target_os = "android", target_os = "macos"))))]
fn compare_xattrs<P: AsRef<Path>>(path1: P, path2: P) -> bool {
Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i will update this code to use the one in
#5835

after this one is merged

let get_sorted_xattrs = |path: P| {
xattr::list(path)
.map(|attrs| {
let mut attrs = attrs.collect::<Vec<_>>();
attrs.sort();
attrs
})
.unwrap_or_else(|_| Vec::new())
};

Check warning on line 3717 in tests/by-util/test_cp.rs

View check run for this annotation

Codecov / codecov/patch

tests/by-util/test_cp.rs#L3708-L3717

Added lines #L3708 - L3717 were not covered by tests

get_sorted_xattrs(path1) == get_sorted_xattrs(path2)
}

Check warning on line 3720 in tests/by-util/test_cp.rs

View check run for this annotation

Codecov / codecov/patch

tests/by-util/test_cp.rs#L3719-L3720

Added lines #L3719 - L3720 were not covered by tests

#[cfg(all(unix, not(any(target_os = "android", target_os = "macos"))))]
#[test]
fn test_acl_preserve() {
use std::process::Command;

let scene = TestScenario::new(util_name!());
let at = &scene.fixtures;
let path1 = "a";
let path2 = "b";
let file = "a/file";
let file_target = "b/file";
at.mkdir(path1);
at.mkdir(path2);
at.touch(file);

let path = at.plus_as_string(file);
// calling the command directly. xattr requires some dev packages to be installed
// and it adds a complex dependency just for a test
match Command::new("setfacl")
.args(["-m", "group::rwx", &path1])
.status()
.map(|status| status.code())
{
Ok(Some(0)) => {}
Ok(_) => {
println!("test skipped: setfacl failed");
return;
}
Err(e) => {
println!("test skipped: setfacl failed with {}", e);

Check warning on line 3751 in tests/by-util/test_cp.rs

View check run for this annotation

Codecov / codecov/patch

tests/by-util/test_cp.rs#L3750-L3751

Added lines #L3750 - L3751 were not covered by tests
return;
}

Check warning on line 3753 in tests/by-util/test_cp.rs

View check run for this annotation

Codecov / codecov/patch

tests/by-util/test_cp.rs#L3753

Added line #L3753 was not covered by tests
}

scene.ucmd().args(&["-p", &path, path2]).succeeds();

Check warning on line 3756 in tests/by-util/test_cp.rs

View check run for this annotation

Codecov / codecov/patch

tests/by-util/test_cp.rs#L3756

Added line #L3756 was not covered by tests

assert!(compare_xattrs(&file, &file_target));

Check warning on line 3758 in tests/by-util/test_cp.rs

View check run for this annotation

Codecov / codecov/patch

tests/by-util/test_cp.rs#L3758

Added line #L3758 was not covered by tests
}
Loading