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

interpreter reference updated to be portable #160

Merged
merged 1 commit into from
Mar 28, 2018
Merged

interpreter reference updated to be portable #160

merged 1 commit into from
Mar 28, 2018

Conversation

tnymlr
Copy link
Contributor

@tnymlr tnymlr commented May 21, 2015

Just change a #!/bin/bash to #!/usr/bin/env bash for a portability. Now runs on OpenBSD without errors.

@karbassi
Copy link
Member

Doing some research on this, here are the pros/cons.

Pros

  • Running a command through /usr/bin/env has the benefit of looking for whatever the default version of the program is in your current environment. This way, you don't have to look for it in a specific place on the system, as those paths may be in different locations on different systems. As long as it's in your path, it will find it.

Cons

  • Since you aren't calling an explicit executable, it's got the potential for mistakes, and on multiuser systems security problems. If someone managed to get their executable called bash in your path, for example.

Thoughts @inkarkat?

@inkarkat
Copy link
Member

I agree with your findings and don't have a clear-cut opinon. Portability should be as high as possible, especially for a universal tool like this, so I'm slightly in favor of it.

@tnymlr
Copy link
Contributor Author

tnymlr commented Mar 28, 2018

I'd say that if somebody replaced bash executable on a system then a person has much bigger security problems than this tool can expose.

@karbassi
Copy link
Member

I'm for this. Merging.

@karbassi karbassi merged commit 14f5de1 into todotxt:master Mar 28, 2018
wwalker pushed a commit to wwalker/todo.txt-cli that referenced this pull request Sep 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants