Skip to content
This repository was archived by the owner on Feb 18, 2021. It is now read-only.

$* will undergo word splitting and pathname expansion #45

Closed
wants to merge 2 commits into from
Closed

$* will undergo word splitting and pathname expansion #45

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
91768d6
Make sure parameters get expanded as passed
andlrc Jul 29, 2016
aee6a3b
Use `/bin/sh` as the code is POSIX compatible
andlrc Jul 29, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions release.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#!/usr/bin/env bash
#!/bin/sh
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with the $@ change but can you explain this one more? When will we have a setup that maintainers don't have bash yet have sh?

Copy link

Choose a reason for hiding this comment

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

I'm inclined to say this PR will hurt more than help: http://stackoverflow.com/questions/10376206/what-is-the-preferred-bash-shebang

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@twolfson @dawsonbotsford

One is guarantied that /bin/sh is on all systems. And since the code is POSIX compatible - and the file is named release.sh - then there is no need to load bash as a faster / more lightweight shell like dash could be symlinked from /bin/sh. Also note that FreeBSD doesn't ship with bash by default

The discussion linked above is concerned about whether or not to use /usr/bin/env sh or simply /bin/sh which makes sense when talking about bash as a user wish to use a newer version of said. But whom are not allowed to change /bin.

But since - as stated before - this code is POSIX compatible the discussion is not as relevant. One could however still argue that they have symlinked /usr/bin/env sh to dash but the system's version is bash in which case the performance boost will be lost. But using /usr/bin/env one extra process needs to be started which also cost something.

Anyway, the main idea behind changing the shebang was that a .sh file should be POSIX compatible and if a shebang is present it should be for the sh executable. I think you can compare it to saving a png file with the .jpg suffix. One could also rename the file to release.bash or release.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we talking about all systems? This script is only for maintainers and we either use Linux Mint (myself) or OS X (mlmorg). I'm pretty sure this is bike shedding and unrelated to the main problem this PR was trying to solve. Please remove this line of code.

# Exit on first error
set -e

# Run foundry with arguments
./node_modules/.bin/foundry release $*
./node_modules/.bin/foundry release "$@"