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

Make 'app list' only return executables #1783

Merged
merged 1 commit into from
Apr 4, 2024

Conversation

foreverigor
Copy link
Contributor

Currently jbang app list prints out all the files in the bin directory. This PR fixes this by filtering the files by the executable flag (scripts in PATH are picked up by the shell only if they're executable anyways)

@foreverigor
Copy link
Contributor Author

Do I need to change the commit name? If required I'll do that

@quintesse
Copy link
Contributor

Do I need to change the commit name? If required I'll do that

If you could, that would be awesome :-)

@quintesse quintesse self-requested a review April 2, 2024 18:46
Copy link
Contributor

@quintesse quintesse left a comment

Choose a reason for hiding this comment

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

Seems like a simple and possibly useful enough change.

@quintesse
Copy link
Contributor

Btw, what is your specific use-case that you need this fix? Theoretically there shouldn't be any non-executable files in that directory, right?

@foreverigor
Copy link
Contributor Author

Btw, what is your specific use-case that you need this fix? Theoretically there shouldn't be any non-executable files in that directory, right?

Theoretically, yes. But in practice any random file will show up, like macOS .DS_Store files (that's how I stumbled on this). Also I could see someone pointing this directory to a different, 'unclean' one where also any file would be seen in the output

@maxandersen maxandersen merged commit 0461c57 into jbangdev:main Apr 4, 2024
5 checks passed
@quintesse
Copy link
Contributor

Btw, for future improvement we should probably change this to use Util.isExecutable() so it will work for Windows too.

@foreverigor
Copy link
Contributor Author

Util.isExecutable() so it will work for Windows too.

TIL, I wasn't aware of this distinction

@quintesse
Copy link
Contributor

TIL, I wasn't aware of this distinction

Yeah, I should have mentioned it, but I forgot. No worries.

Things is that on Windows Files.isExecutable() always returns true because Windows doesn't really have a flag or anything that determines if something is executable or not, there's just a list of extensions that can be considered executable but the JDK function doesn't deal with that. So we wrote our own that looks at the extension on Windows and uses File.isExecutable for everything else.

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.

3 participants