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

build: do not require the nonstandard and unpredictable 'which' utility #811

Merged
merged 1 commit into from
Dec 13, 2019
Merged

build: do not require the nonstandard and unpredictable 'which' utility #811

merged 1 commit into from
Dec 13, 2019

Conversation

eli-schwartz
Copy link
Contributor

This may not be installed on various systems, and it's difficult to test for the availability of the tool you need, if the check program itself does not exist.

The POSIX 2008 specification mandates the command -v builtin; bash is a POSIX 2008 compliant shell, and this builtin has worked since bash 1.x anyway.

A side benefit of using the POSIX portable option is that it requires neither an external disk executable, nor (because unlike "which", the exit code is reliable) a subshell fork. This therefore represents a mild speedup.

This may not be installed on various systems, and it's difficult to test
for the availability of the tool you need, if the check program itself
does not exist.

The POSIX 2008 specification mandates the `command -v` builtin; bash is
a POSIX 2008 compliant shell, and this builtin has worked since bash 1.x
anyway.

A side benefit of using the POSIX portable option is that it requires
neither an external disk executable, nor (because unlike "which", the
exit code is reliable) a subshell fork. This therefore represents a mild
speedup.
@eli-schwartz
Copy link
Contributor Author

Because I don't have which installed on my system, so make install failed.

@eli-schwartz
Copy link
Contributor Author

Totally as a side note, the script doesn't actually make use of bash-specific features, so it could be modified to run via /bin/sh; it's not the only script in that situation.

@spacewander spacewander merged commit 69e96cb into tj:master Dec 13, 2019
@spacewander
Copy link
Collaborator

@eli-schwartz
Merged. Thank you!

We only target to bash: https://github.com/tj/git-extras/blob/master/CONTRIBUTING.md#your-new-git-extra-command-should-support
Some scripts don't use bash-specific features but some do. Enforce /usr/bin/env bash keeps the consistence, so that we don't need to remember which script is bash-only while which is not.

@eli-schwartz eli-schwartz deleted the portability branch December 13, 2019 02:30
@eli-schwartz
Copy link
Contributor Author

Yeah, probably makes sense I suppose. I'm just a persnickety /bin/sh script writer. :D

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