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

mv: preserve the xattr #5835

Merged
merged 6 commits into from
Jan 16, 2024
Merged

mv: preserve the xattr #5835

merged 6 commits into from
Jan 16, 2024

Conversation

sylvestre
Copy link
Contributor

Will conflict with
#5834
but will be easy to fix

Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/mv/acl is no longer failing!

Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/cp/acl is no longer failing!
Congrats! The gnu test tests/mv/acl is no longer failing!
Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

@cakebaker
Copy link
Contributor

I don't know whether you have seen it, test_cp_preserve_xattr_fails_on_android fails on Android.

@@ -52,6 +52,7 @@ use unicode_width::UnicodeWidthStr;
use uucore::libc::{dev_t, major, minor};
#[cfg(unix)]
use uucore::libc::{S_IXGRP, S_IXOTH, S_IXUSR};
#[cfg(any(not(unix), target_os = "android", target_os = "macos"))]
Copy link
Contributor

Choose a reason for hiding this comment

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

This cfg seems unnecessary if you remove line_ending::LineEnding from line 67

@sylvestre
Copy link
Contributor Author

I don't know whether you have seen it, test_cp_preserve_xattr_fails_on_android fails on Android.

yeah, I am waiting for this PR to land before working on it :)

tests/by-util/test_mv.rs Outdated Show resolved Hide resolved
Comment on lines +637 to +639
#[cfg(all(unix, not(target_os = "macos")))]
let xattrs =
fsxattr::retrieve_xattrs(from).unwrap_or_else(|_| std::collections::HashMap::new());
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there some reason why xattrs is set here and not after the following if-block? Otherwise you could put this snippet, and the one on line 652, into one block and save one cfg (and show that they belong together).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I would have to duplicate the declaration as the xattr detection needs to happen with and without the progress bar

and you can't do at the same time because "from" gets removed/moved :)

so, to rephrase

  1. get the attributes on the file
  2. move the file
  3. set the attributes on the "new" file

Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/mv/acl is no longer failing!

Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/mv/acl is no longer failing!

Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/mv/acl is no longer failing!

@cakebaker cakebaker merged commit 55b7b2f into uutils:main Jan 16, 2024
59 of 61 checks passed

let test_attr = "user.test_attr";
let test_value = b"test value";
xattr::set(&file_path1, test_attr, test_value).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

This line triggers the following error on my machine:

thread 'common::util::tests::test_compare_xattrs' panicked at tests/common
called `Result::unwrap()` on an `Err` value: Os { code: 95, kind: Uncategorized, message: "Operation not supported" }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

linux ? mac?

Copy link
Member

Choose a reason for hiding this comment

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

On Linux

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you have apparmor or selinux?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think so 🤔

@sylvestre sylvestre deleted the mv-acl branch January 16, 2024 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants