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

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

Closed
wants to merge 3 commits into from

Conversation

vvuk
Copy link

@vvuk vvuk commented Feb 9, 2021

Doing this on Windows is very slow; this adds a feature flag to disable doing these checks, making lsd as fast as any other directory listing tool. Without this, running lsd on a directory with ~100 files takes around 600ms. With this feature enabled, it takes 40ms.

Adds a bunch of compilation warnings unfortunately; the structure could be better to clean this up.


TODO

  • Use cargo fmt
  • Add necessary tests
  • Add changelog entry

@codecov-io
Copy link

Codecov Report

Merging #484 (7526910) into master (841ad99) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #484   +/-   ##
=======================================
  Coverage   84.45%   84.45%           
=======================================
  Files          36       36           
  Lines        3378     3378           
=======================================
  Hits         2853     2853           
  Misses        525      525           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 841ad99...7526910. Read the comment docs.

@meain
Copy link
Member

meain commented Feb 13, 2021

Having this behind a feature flag seems like a good compromise until we have a better solution, which I guess should be possible from #475 (comment)

One thing that's kind of interesting is that dir /q can pull this info quite quickly. cmd /c dir /q takes about 50ms for my test dir from another PR (unpatched lsd takes about 600ms; patched to hack out permission/ownership, 40ms). That pays the same process creation cost (cmd.exe) as lsd would. Might be useful to systrace & profile cmd.exe and see what it's doing, as dir is an internal call there.

What do you think about having all the values as false instead of true?

@vvuk
Copy link
Author

vvuk commented Feb 13, 2021

false works for me, done!

Copy link
Member

@meain meain left a comment

Choose a reason for hiding this comment

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

Hey, just a few changes. Plus could also handle/suppress the unnecessary warnings.

group_write: false,
group_execute: false,

other_read: true,
Copy link
Member

Choose a reason for hiding this comment

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

We could make this as false as well.

@@ -13,6 +13,32 @@ use super::{Owner, Permissions};

const BUF_SIZE: u32 = 256;

#[cfg(feature = "no-windows-permissions")]
pub fn get_file_data(path: &Path) -> Result<(Owner, Permissions), io::Error> {
let owner = Owner::new("?".to_string(), "?".to_string());
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to make owner optional in the struct in case of windows(we could probably have #473 added in after that).
Also have the user/group as - with a greyish colors simliar to when there are no permissions. Something like this.
Screenshot 2021-02-13 at 8 45 14 PM

Copy link
Author

Choose a reason for hiding this comment

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

That's a great idea -- I'll get on it today. I may split the no-windows-perms stuff into a separate .rs file, so that it's easier to cfg out stuff to avoid warnings too.

Copy link
Author

Choose a reason for hiding this comment

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

(sorry, life got in the way this past week. will get to it when I can!)

@Aankhen
Copy link

Aankhen commented Oct 5, 2021

I tried this locally combined with #473 (after fixing a tiny merge conflict) and it makes a huge difference. Running lsd in a particular directory consistently took about 8 seconds before. Now it takes 479 milliseconds.

@zwpaper zwpaper mentioned this pull request Mar 2, 2023
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.

I have one more question, as this is added as a build-time feature, should we need to release a new binary for this feature?

@meain

and, @vvuk did you have some cycle to update this PR again, and we can make it happen after the discuss resolved?

or I can help to send a patch if you did not have time.

Comment on lines +217 to +220
#[cfg(all(windows, feature = "no-windows-permissions"))]
let (owner, permissions) = (None, None);

#[cfg(windows)]
#[cfg(all(windows, not(feature = "no-windows-permissions")))]
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 checking this feature inside windows_utils.rs, so that we can keep checking unix/windows and drop the necessary to set owner and permissions as Optional, and keep the logic of Windows inside windows_utils.rs?

@meain

@@ -37,10 +44,18 @@ impl<'a> From<&'a Metadata> for Owner {

impl Owner {
pub fn render_user(&self, colors: &Colors) -> ColoredString {
colors.colorize(self.user.clone(), &Elem::User)
if self.user.len() == 0 {
colors.colorize("?".to_owned(), &Elem::User)
Copy link
Member

Choose a reason for hiding this comment

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

as #484 (comment) mentioned, this should be -, right?

@meain

@vvuk
Copy link
Author

vvuk commented Mar 8, 2023

and, @vvuk did you have some cycle to update this PR again, and we can make it happen after the discuss resolved?

I don't at the moment -- if you're able to take it over, please do!

@domsleee
Copy link
Contributor

I suggest closing this one since #882 is merged (adding --permission disabled) 👍

@zwpaper zwpaper closed this Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants