✨ add disable option for permission #882

zwpaper · 2023-08-28T11:59:08Z

add an option for permission args to disable the detection of owner and permission,
it may be used in Windows to speed up.

a temporary fix for #200

this is a POC or RFC, any comment is welcome!

TODO

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

muniu-bot · 2023-08-28T11:59:19Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: zwpaper

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

Needs approval from an approver in each of these files:

OWNERS

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

domsleee · 2023-09-15T12:54:41Z

This is what the output looks like

❯ cargo run -- --long
    Finished dev [unoptimized + debuginfo] target(s) in 0.11s
     Running `target\debug\lsd.exe --long`
lsd: .: The specified domain either does not exist or could not be contacted. (os error 1355)

lsd: .\build.rs: The specified domain either does not exist or could not be contacted. (os error 1355)

lsd: .\Cargo.lock: The specified domain either does not exist or could not be contacted. (os error 1355)

lsd: .\Cargo.toml: The specified domain either does not exist or could not be contacted. (os error 1355)

lsd: .\CHANGELOG.md: The specified domain either does not exist or could not be contacted. (os error 1355)

lsd: .\ci: The specified domain either does not exist or could not be contacted. (os error 1355)

lsd: .\CODEOWNERS: The specified domain either does not exist or could not be contacted. (os error 1355)

lsd: .\doc: The specified domain either does not exist or could not be contacted. (os error 1355)

lsd: .\LICENSE: The specified domain either does not exist or could not be contacted. (os error 1355)

lsd: .\README.md: The specified domain either does not exist or could not be contacted. (os error 1355)

lsd: .\rustfmt.toml: The specified domain either does not exist or could not be contacted. (os error 1355)

lsd: .\src: The specified domain either does not exist or could not be contacted. (os error 1355)

lsd: .\target: The specified domain either does not exist or could not be contacted. (os error 1355)

lsd: .\tests: The specified domain either does not exist or could not be contacted. (os error 1355)

.????????? ? ? 1.8 KB Tue Sep 12 14:47:32 2023  build.rs
.????????? ? ?  39 KB Fri Sep 15 22:24:29 2023  Cargo.lock
.????????? ? ? 1.9 KB Fri Sep 15 22:24:29 2023  Cargo.toml
.????????? ? ?  20 KB Fri Sep 15 22:24:29 2023  CHANGELOG.md
d????????? ? ?   0 B  Tue Sep 12 14:47:32 2023  ci
.????????? ? ?  35 B  Tue Sep 12 14:47:32 2023  CODEOWNERS
d????????? ? ?   0 B  Tue Sep 12 14:47:32 2023  doc
.????????? ? ?  11 KB Tue Sep 12 14:47:32 2023  LICENSE
.????????? ? ?  20 KB Fri Sep 15 22:24:29 2023  README.md
.????????? ? ? 181 B  Tue Sep 12 14:47:32 2023  rustfmt.toml
d????????? ? ? 4.0 KB Fri Sep 15 22:24:29 2023  src
d????????? ? ?   0 B  Tue Sep 12 14:48:03 2023  target
d????????? ? ?   0 B  Tue Sep 12 14:47:32 2023  tests

Perhaps we can provide a hint to the user, e.g.

This can be caused by network issues when contacting a remote domain, try disabling permissions with --permission disable

I'm still thinking the mode attributes should be default for windows (#866 (comment)), ACL seems like a bad default because it isn't safe
Like --permission could be

rwx (default); which is rwx on linux and mode attributes on windows
octal: octal on linux, mode attributes on windows

Then we could have a separate --permission-windows-acl flag as an unsafe option, e.g. for users not on a domain or with an ACL setup that doesn't require files from a remote machine.

zwpaper · 2023-09-15T16:47:28Z

hi @domsleee, thanks for the advice!

Honestly, I am not a Windows user, so your suggestion(or other Windows users) would be valuable to me.

I was trying to add the ability to disable permission detection on Windows in this PR, first, let me make sure that this works in your view.

and for the default options for windows, how about we try and disable it automatically if errors occur, this seems more to be intelligent.

domsleee · 2023-09-16T03:59:20Z

src/meta/mod.rs

+            match windows_utils::get_file_data(path) {
+                Ok((owner, permissions)) => (Some(owner), Some(permissions)),
+                Err(e) => {
+                    eprintln!("lsd: {}: {}\n", path.to_str().unwrap_or(""), e);


I think a hint here could help users:

❯ lsd let hint = if e.raw_os_error() == Some(ERROR_NO_SUCH_DOMAIN) { " Hint: Consider using `--permission disable`." } else { "" };

Outputs:

lsd: .: The specified domain either does not exist or could not be contacted. (os error 1355). Hint: Consider using `--permission disable`. lsd: .\build.rs: The specified domain either does not exist or could not be contacted. (os error 1355). Hint: Consider using `--permission disable`. lsd: .\Cargo.lock: The specified domain either does not exist or could not be contacted. (os error 1355). Hint: Consider using `--permission disable`. lsd: .\Cargo.toml: The specified domain either does not exist or could not be contacted. (os error 1355). Hint: Consider using `--permission disable`.

domsleee · 2023-09-16T04:01:48Z

I was trying to add the ability to disable permission detection on Windows in this PR, first, let me make sure that this works in your view.

From my perspective, the --permission disable fixes the performance issue and stops the errors coming up, so this is a good fix 👍

and for the default options for windows, how about we try and disable it automatically if errors occur, this seems more to be intelligent.

Yeah, it could be an idea to show only the first time 1355 occurs, so that the output is less noisy.

There are two cases

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

Case 1 is fixed by the error handling in your PR, but case 2 is why I'm suggesting that ACL shouldn't be default 👍

zwpaper · 2023-09-18T15:25:52Z

hi @domsleee, it sounds reasonable to not make it as default for Windows, let me finish this PR, and maybe you can try to implement the rest two?

and you could create 2 issues to track them.

Signed-off-by: Wei Zhang <[email protected]>

codecov-commenter · 2023-09-19T05:55:09Z

Codecov Report

Attention: Patch coverage is 82.45614% with 20 lines in your changes missing coverage. Please review.

Project coverage is 85.75%. Comparing base (bfbb217) to head (53e333d).
Report is 76 commits behind head on master.

Files with missing lines	Patch %	Lines
src/meta/mod.rs	61.11%	14 Missing ⚠️
src/core.rs	0.00%	4 Missing ⚠️
src/meta/owner.rs	33.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #882      +/-   ##
==========================================
- Coverage   85.93%   85.75%   -0.18%     
==========================================
  Files          51       51              
  Lines        4962     5005      +43     
==========================================
+ Hits         4264     4292      +28     
- Misses        698      713      +15

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

zwpaper requested review from meain and Peltoche as code owners August 28, 2023 11:59

zwpaper added this to the v1.1.0 milestone Sep 15, 2023

zwpaper mentioned this pull request Sep 15, 2023

Add windows-acl-permissions feature to fix perf issue on windows #902

Closed

5 tasks

domsleee reviewed Sep 16, 2023

View reviewed changes

zwpaper added 2 commits September 19, 2023 13:32

✨ add disable option for permission

842c576

Signed-off-by: Wei Zhang <[email protected]>

🔍 🔨 fix test for adding disable permission

53e333d

Signed-off-by: Wei Zhang <[email protected]>

zwpaper force-pushed the dev-disable-permission branch from f5e7bcc to 53e333d Compare September 19, 2023 05:51

muniu-bot bot added the size/L label Sep 19, 2023

zwpaper merged commit 177c910 into lsd-rs:master Sep 20, 2023
18 checks passed

This was referenced Sep 24, 2023

WIndows 10: App does not work #375

Closed

Add no-windows-permissions feature, to disable access/owner checks #484

Closed

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

Merged

branchvincent mentioned this pull request Mar 22, 2024

lsd 1.1.0 Homebrew/homebrew-core#166912

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ add disable option for permission #882

✨ add disable option for permission #882

zwpaper commented Aug 28, 2023

muniu-bot bot commented Aug 28, 2023

domsleee commented Sep 15, 2023

zwpaper commented Sep 15, 2023

domsleee Sep 16, 2023 •

edited

Loading

domsleee commented Sep 16, 2023

zwpaper commented Sep 18, 2023

codecov-commenter commented Sep 19, 2023 •

edited by codecov bot

Loading

✨ add disable option for permission #882

✨ add disable option for permission #882

Conversation

zwpaper commented Aug 28, 2023

TODO

muniu-bot bot commented Aug 28, 2023

domsleee commented Sep 15, 2023

zwpaper commented Sep 15, 2023

domsleee Sep 16, 2023 • edited Loading

Choose a reason for hiding this comment

domsleee commented Sep 16, 2023

zwpaper commented Sep 18, 2023

codecov-commenter commented Sep 19, 2023 • edited by codecov bot Loading

Codecov Report

domsleee Sep 16, 2023 •

edited

Loading

codecov-commenter commented Sep 19, 2023 •

edited by codecov bot

Loading