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

scripts: Use bash-builtin command -v instead of external which #555

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

a1346054
Copy link
Contributor

command -v is a bash builtin and is a standardized version of which

`command -v` is a bash builtin and is a standardized version of `which`
Copy link
Contributor

@hartwork hartwork left a comment

Choose a reason for hiding this comment

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

Hi @a1346054,

I checked all four files for Bash shebang lines and they all have one (while not the most portable version) so that requirement is filled. However I'm surprised to see command -v in the new version rather than type -P, the reason being that if our intention is replacing which by something that is equivalent but faster, the fact that command -v would also recognize (and prefer) Bash functions of the requested name seems like a mis-feature. Let me demonstrate:

# command -v curl
/usr/bin/curl

# type -P curl
/usr/bin/curl

# curl() { :; }

# command -v curl
curl

# type -P curl
/usr/bin/curl

So I'd consider use of type -P more robust a closer match. What do you think?

Best, Sebastian

PS: This is just my 2 cents and I don't have write/merge permissions in this repository.

@a1346054
Copy link
Contributor Author

The behavior of command -v regarding functions and aliases should not be a problem because user-defined functions and aliases don't persist into scripts.

Additionally, command -v is not just a bash builtin, it's defined by POSIX, so it behaves the same everywhere.

type -P is not POSIX

@hartwork
Copy link
Contributor

The behavior of command -v regarding functions and aliases should not be a problem because user-defined functions and aliases don't persist into scripts.

True, but someone could add functions right into these scripts. I'm not saying it's super likely to happen but command is not a fit in that regard. It's not a drop-in replacment to which.

Additionally, command -v is not just a bash builtin, it's defined by POSIX, so it behaves the same everywhere.

type -P is not POSIX

Our target is Bash, not POSIX.

@a1346054
Copy link
Contributor Author

a1346054 commented Sep 13, 2022

True, but someone could add functions right into these scripts.

then someone could also do type() { echo "something"; }; and break stuff, so we'd need to protect it by calling:

builtin type -P curl

but it gets really crazy if we assume the scenario that users will edit the provided scripts in crazy ways.

Wonder how to fix it if the user did something like:

type() { echo "something"; };
builtin() { echo "anything"; };

@hartwork
Copy link
Contributor

True, but someone could add functions right into these scripts.

then someone could also do type() { echo "something"; }; and break stuff, so we'd need to protect it by calling:

builtin type -P curl

It doesn't seem to stop there, even builtin() { :; } seems possible, just tried with Bash 5.1.16.

but it gets really crazy if we assume the scenario that users will edit the provided scripts in crazy ways.

I was not speaking of end users. I meant people adjusting these scripts in Git upstream over time.

I think we have both made our points. I would use type -P over command -v because it's more to the point and because we have Bash features available. We can leave the decision to someone with actual merge permissions. @Cropi @radosroka what do you think?

@a1346054
Copy link
Contributor Author

I was not speaking of end users. I meant people adjusting these scripts in Git upstream over time.

In that case, it might be of interest to note that the behavior of command -v curl is correct there.

If some maintainer for example does:

curl() { wget "$@"; };

with the intention that calling curl is now overridden, then command -v curl will respect that whereas type -P curl will do something else that the maintainer might not have realized.

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.

2 participants