Skip to content

mv: preserve the xattr#5835

Merged
cakebaker merged 6 commits intouutils:mainfrom
sylvestre:mv-acl
Jan 16, 2024
Merged

mv: preserve the xattr#5835
cakebaker merged 6 commits intouutils:mainfrom
sylvestre:mv-acl

Conversation

@sylvestre
Copy link
Contributor

Will conflict with
#5834
but will be easy to fix

@github-actions
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!

@github-actions
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.

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 :)

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

@github-actions
Copy link

GNU testsuite comparison:

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

@github-actions
Copy link

GNU testsuite comparison:

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

@github-actions
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

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

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
Collaborator

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
Collaborator

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