[go: up one dir, main page]
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.

Sign up for GitHub

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

Jump to bottom

show @ if extended attributes are present #962

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

show @ if extended attributes are present #962

wants to merge 1 commit into from

Conversation

accidentaldevelopment
Copy link

ls -l on macOS will show an @ if the file or directory has extended attributes. This PR adds the same functionality to lsd.

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

❯ cargo r -- -l
    Finished dev [unoptimized + debuginfo] target(s) in 0.05s
     Running `target/debug/lsd -l`
.rw-r--r--@ user group 1.7 KB Thu Nov  9 16:27:32 2023  build.rs
.rw-r--r--@ user group  39 KB Thu Nov  9 16:27:32 2023  Cargo.lock
   ...omitted
drwxr-xr-x@ user group 192 B  Thu Nov  9 16:32:28 2023  target
drwxr-xr-x@ user group  96 B  Thu Nov  9 16:27:32 2023 󰙨 tests

Concerns

This adds a new has_xattr field to the AccessControl struct. That field is determined by checking selinux_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

  • Use cargo fmt
  • Add necessary tests
  • Update default config/theme in README (if applicable)
  • Update man page at lsd/doc/lsd.md (if applicable)

@muniu-bot muniu-bot bot added the size/M label Dec 1, 2023
@muniu-bot Muniu Bot
Copy link
muniu-bot bot commented Jan 25, 2024

[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.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@zwpaper
Copy link
Member
zwpaper commented Feb 5, 2024

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

@accidentaldevelopment
Copy link
Author
accidentaldevelopment commented Feb 6, 2024 •
edited
Loading

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.

@zwpaper
Copy link
Member
zwpaper commented Feb 6, 2024

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.

@CarterLi
Copy link

Maybe adding an option to control it is a better idea?

@accidentaldevelopment
Copy link
Author

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:

  1. Hide the entire 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.
  2. In render_method, just ignore has_xattr if we're not on macOS. Little awkward in the if statement, but otherwise pretty straight-forward.
  3. In 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@meain meain Awaiting requested review from meain

@Peltoche Peltoche Awaiting requested review from Peltoche

@zwpaper zwpaper Awaiting requested review from zwpaper zwpaper is a code owner

Assignees
No one assigned
Labels
size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@accidentaldevelopment @zwpaper @CarterLi