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

Fix showing every file as executable on Windows #769

Merged
merged 4 commits into from Nov 26, 2022
Merged

Conversation

Icxolu
Copy link
Contributor

@Icxolu Icxolu commented Nov 15, 2022

This PR determines the executable state of a file on Windows using the file extension instead of permissions.
This fixes that every regular file was marked executable (green).

  • added --exec-ext <ext> to change mark extensions as executable on the fly
  • added executable-extensions config to modify them more permantly LS_COLORS can be used to customize (other) extentions
  • cli flags and config file options will be merged
  • exe, msi, cmd and ps1 will be marked as executable on windows

Fixes #712


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)

@meain
Copy link
Member

meain commented Nov 23, 2022

I like the idea of having predefined file extensions being marked as executable. I don't think we should have an option to configure it though. If they want to add more file extensions to be marked as green, one can always control it with LS_COLORS(or PR to upstream if it is really common). Could you rework it so that we don't have a new cli flag or config option?

`exe`, `msi`, `bat` and `ps1` are marked as executable.
@Icxolu
Copy link
Contributor Author

Icxolu commented Nov 23, 2022

I dropped the configuration option and cli flag. I was not really happy with them either, it felt like a little too much, but i haven't thought of just using LS_COLORS as the customizable option. For now i marked exe, msi, bat and ps1 as executable.

@Icxolu
Copy link
Contributor Author

Icxolu commented Nov 23, 2022

With this in place, we could probably also revisit fetching ACL permission only on demand, since we don't use any other Permission info for coloring. This could potentially improve performance on (domain connected) Windows machines significantly. As a replacement, we could introduce a Mode block, like PowerShell does, which displays the file attributes.

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.

Looks good. Add a test for verifying this behavior on windows and we should be good to go. :D

@@ -127,6 +127,7 @@ impl Permissions {
ColoredString::new(Colors::default_style(), res)
}

#[cfg_attr(windows, allow(dead_code))]
Copy link
Member

Choose a reason for hiding this comment

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

Nit, we can change this to #[cfg(not(windows))].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done 😇

@codecov-commenter
Copy link

Codecov Report

Merging #769 (f42409e) into master (f5ee0a2) will decrease coverage by 0.07%.
The diff coverage is 11.11%.

@@            Coverage Diff             @@
##           master     #769      +/-   ##
==========================================
- Coverage   86.68%   86.60%   -0.08%     
==========================================
  Files          44       44              
  Lines        4332     4336       +4     
==========================================
  Hits         3755     3755              
- Misses        577      581       +4     
Impacted Files Coverage Δ
src/meta/mod.rs 43.16% <0.00%> (-0.32%) ⬇️
src/meta/permissions.rs 92.56% <ø> (ø)
src/meta/filetype.rs 80.55% <12.50%> (-2.31%) ⬇️
src/theme/icon.rs 100.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@meain meain merged commit c48f0f4 into lsd-rs:master Nov 26, 2022
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.

Stop lsd on windows from showing every regular file as green
3 participants