search

lsd-rs / lsd

The next gen ls command
Apache License 2.0
13.44k stars 436 forks source link

:sparkles: add disable option for permission #882

Closed zwpaper closed 1 year ago

zwpaper commented 1 year ago

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 https://github.com/lsd-rs/lsd/issues/200

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

TODO

muniu-bot[bot] commented 1 year ago

[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](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 1 year ago

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 (https://github.com/lsd-rs/lsd/issues/866#issuecomment-1721012397), ACL seems like a bad default because it isn't safe Like --permission could be

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 commented 1 year ago

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 commented 1 year ago

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 1) GetEffectiveRightsFromAclW can't access the domain, and os error 1355 is thrown 2) 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 commented 1 year ago

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.

codecov-commenter commented 1 year ago

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 :warning:
src/core.rs 0.00% 4 Missing :warning:
src/meta/owner.rs 33.33% 2 Missing :warning:
Additional details and impacted files ```diff @@ 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 ```

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