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.

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

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

Merged

Conversation

domsleee
Copy link
Contributor

@domsleee domsleee commented Sep 25, 2023

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 #882 (comment):

  • 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

  • powershell uses it in Get-ChildItem
  • eza uses it as the default for windows (eza -al)

Checklist

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

Screenshot & comparison

screenshot

@muniu-bot
Copy link

muniu-bot bot commented Sep 25, 2023

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

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

@domsleee domsleee force-pushed the feature/windows-default-permissions-attributes branch from cc5f026 to d88a55f Compare September 29, 2023 09:31
@domsleee domsleee marked this pull request as ready for review September 29, 2023 09:41
@domsleee
Copy link
Contributor Author

domsleee commented Oct 2, 2023

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
Copy link

roqvist commented Jan 10, 2024

Anyway to help this forward?

@zwpaper
Copy link
Member

zwpaper commented Jan 10, 2024

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

/assign
/milestone v1.1.0

@zwpaper zwpaper added this to the v1.1.0 milestone Jan 10, 2024
Copy link
Member

@zwpaper zwpaper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@domsleee really sorry for the late reply, thanks so much for the great work! it LGTM mostly but only a nit

src/meta/mod.rs Outdated
mod size;
mod symlink;
mod windows_attributes;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about #[cfg(windows)] here, and drop the #cfg inside windows_attributes?

@@ -0,0 +1,126 @@
#[cfg(windows)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can drop all the #[cfg(windows)] in this file, if we set it in mod.rs

Signed-off-by: Wei Zhang <kweizh@gmail.com>
@zwpaper
Copy link
Member

zwpaper commented Feb 16, 2024

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

Copy link

codecov bot commented Feb 16, 2024

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 ⚠️
src/meta/mod.rs 65.62% 11 Missing ⚠️
src/core.rs 0.00% 7 Missing ⚠️
src/color.rs 20.00% 4 Missing ⚠️
src/meta/permissions_or_attributes.rs 40.00% 3 Missing ⚠️
src/flags/permission.rs 88.88% 1 Missing ⚠️
src/meta/filetype.rs 80.00% 1 Missing ⚠️
src/meta/permissions.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zwpaper zwpaper merged commit b4d4462 into lsd-rs:master Feb 16, 2024
18 of 20 checks passed
@domsleee
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lsd is slow Windows
3 participants