Open accidentaldevelopment opened 7 months ago
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: accidentaldevelopment
The full list of commands accepted by this bot can be found here.
hi @accidentaldevelopment, may I ask whether this is the macOS-only feature, not the GNU ls?
if so, we should leave it macOS-only.
if there will later be some linux requirements, we could put it behind a flag, and should not enable by default in linux
Hello, @zwpaper, thanks for the reply!
The answer to your question is exactly why this one could be a little ugly. Linux currently checks for specific xattrs (system.posix_acl_access, security.selinux) to determine if it should show .
or +
. However macOS will show @
if there are any xattrs on the file.
ls
on Linux, to my knowledge, doesn't give any particular indication for arbitrary xattrs, just the few known ones that lsd
already shows. While macOS seems to just have the single catchall for any xattrs.
Related is a macOS-specific flag -@
to list those xattrs. That functionality is not part of this PR.
Does that help? It's kind of a weird divergence between the two OS families.
hi @accidentaldevelopment, thanks for the explanation, the problem is that it will break the linux users if the @
appear by default especially when we were trying to align with the GNU ls.
so I would prefer showing @ only on macOS, so we will not break any linux use-cases.
Maybe adding an option to control it is a better idea?
Whoops! Sorry, @zwpaper – I missed the notification for the replies!
so I would prefer showing @ only on macOS, so we will not break any linux use-cases.
Makes sense. There are a few different ways I could think of to do this:
has_xattr
field behind a cfg
so it only exists on macOS. This may be the most correct indication on a given OS, but it would also make for several extra cfg
blocks to detect and print.render_method
, just ignore has_xattr
if we're not on macOS. Little awkward in the if
statement, but otherwise pretty straight-forward.AccessControl::from_path
, always set has_xattr
to false
unless we're on macOS. Probably the simplest since all other logic is the same - it's just always false
on linux. Avoids specialized tests as well. But it's an awkward implementation detail existing in a function. If any other constructor functions are added, they'd have to follow the same rules.I'm open to any of the above, or other options I may have missed. Obviously I would want to do whatever is best for the codebase and experience!
Maybe adding an option to control it is a better idea?
I'm a little unsure what you mean here, @CarterLi. Are you suggesting it goes behind a command line flag? This would break compatibility with ls
on macOS, and be rather unintuitive I think.
Or are you referring to a config file option? That could probably work quite well, and default to true
on macOS.
ls -l
on macOS will show an@
if the file or directory has extended attributes. This PR adds the same functionality tolsd
.macOS also includes a
-@
flag to display the extended attributes. This PR does not include that functionality. If it's something that's wanted, it should probably be done in a followup PR.Example for this repo
Concerns
This adds a new
has_xattr
field to theAccessControl
struct. That field is determined by checkingselinux_context
,smack_context
, and just listing xattrs.If my understanding and testing are correct, this should never really show up in anything other than macOS. But the code and field will be present anyway. It feels a little dirty having a field that will never really be used outside of a single platform. But the alternative is some conditional compilation, and I generally feel that should be approached with care.
Happy to discuss alternative implementations!
TODO
cargo fmt