lsd-rs / lsd

The next gen ls command
Apache License 2.0
13.05k stars 425 forks source link

feat: Windows safe default permissions (fixes ACL errors/performance) #911

Closed domsleee closed 6 months ago

domsleee commented 11 months ago

Fixes: #200 by implementing #866 as the default permission setting for windows.

Context

The current default (--permission rwx) uses ACL on windows, which can throw errors and has performance problems, described in #200 and also in https://github.com/lsd-rs/lsd/pull/882#issuecomment-1722126112:

  • GetEffectiveRightsFromAclW can't access the domain, and os error 1355 is thrown
  • GetEffectiveRightsFromAclW works, but it can be very slow when the information about permissions requires files from a remote server [1 second per file for me, worse for others]

Suggestion

  1. A new --permission attributes is introduced
  2. It is used as the default for windows

Prior art

Checklist

Screenshot & comparison

screenshot

muniu-bot[bot] commented 11 months ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: domsleee

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/lsd-rs/lsd/blob/master/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
domsleee commented 11 months ago

btw - I'm not sure why the work-in-progress label is still on this PR. I think muniu-bot should have removed it when I marked the PR is ready, or perhaps I missed something 😄

Could you please review this one when convenient @zwpaper

roqvist commented 7 months ago

Anyway to help this forward?

zwpaper commented 7 months ago

sorry for the late reply, I will look into this recently

/assign /milestone v1.1.0

zwpaper commented 6 months ago

hi @domsleee, as it has been some time since you responded, I have pushed a commit to make this PR go forward.

hope you don't mide

codecov[bot] commented 6 months ago

Codecov Report

Attention: 46 lines in your changes are missing coverage. Please review.

Comparison is base (1714d89) 85.74% compared to head (281c74a) 84.63%. Report is 23 commits behind head on master.

Files Patch % Lines
src/meta/windows_attributes.rs 0.00% 18 Missing :warning:
src/meta/mod.rs 65.62% 11 Missing :warning:
src/core.rs 0.00% 7 Missing :warning:
src/color.rs 20.00% 4 Missing :warning:
src/meta/permissions_or_attributes.rs 40.00% 3 Missing :warning:
src/flags/permission.rs 88.88% 1 Missing :warning:
src/meta/filetype.rs 80.00% 1 Missing :warning:
src/meta/permissions.rs 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #911 +/- ## ========================================== - Coverage 85.74% 84.63% -1.11% ========================================== Files 51 53 +2 Lines 5003 5078 +75 ========================================== + Hits 4290 4298 +8 - Misses 713 780 +67 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

domsleee commented 6 months ago

Hi @zwpaper, sorry I missed your review - thanks for reviewing and helping out with this PR 🎉