-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
mv: preserve the xattr #5835
Conversation
GNU testsuite comparison:
|
6b4fd4c
to
fdd2591
Compare
GNU testsuite comparison:
|
I don't know whether you have seen it, |
src/uu/ls/src/ls.rs
Outdated
@@ -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"))] |
There was a problem hiding this comment.
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
yeah, I am waiting for this PR to land before working on it :) |
#[cfg(all(unix, not(target_os = "macos")))] | ||
let xattrs = | ||
fsxattr::retrieve_xattrs(from).unwrap_or_else(|_| std::collections::HashMap::new()); |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
- get the attributes on the file
- move the file
- set the attributes on the "new" file
GNU testsuite comparison:
|
GNU testsuite comparison:
|
used by cp & mv (at least)
Should make tests/mv/acl pass
GNU testsuite comparison:
|
|
||
let test_attr = "user.test_attr"; | ||
let test_value = b"test value"; | ||
xattr::set(&file_path1, test_attr, test_value).unwrap(); |
There was a problem hiding this comment.
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" }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
linux ? mac?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Linux
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 🤔
Will conflict with
#5834
but will be easy to fix