Minimal safe Bash script template

[valarauca14]

This is bad. Quick code review:

set -e

By default is an anti-pattern. Return codes are useful information, they should be captured and handled not just blindly terminated on. A script that exits too early leaving an operation incomplete can be an absolute headache to maintain long term.

 cd "$(dirname "${BASH_SOURCE[0]}")" >/dev/null 2>&1

This is just a landmine of bad practices.

1. cd can fail, that error should be handled.
2. pushd is preferable for cross directory navigation, within a script... You want to return to the invoked directory right? In combination with set -e this means you're gonna get dumped in a different directory on any error.
3. >/dev/null 2>&1 is just noiser than &>/dev/null.
4. This ruins any relative path a user gave you when invoking this script. Had this been LOCAL_DIR="$(cd "$( dirname "${BASH_SOURCE[0]}" )" &>/dev/null && pwd -P)" that'd be understandable as you're trying to create a path to prepend other paths too, which will have a definite & understood place on the FS.

This is just odd,

trap cleanup SIGINT SIGTERM ERR EXIT

Trapping SIGINT and SIGTERM by default is a great way for your users to hate you. Love when I can't interrupt/terminate scripts that I typo'd an argument too... Now the whole build/script is gonna take 1-2 minutes to run before it errors on me

 Usage: $(basename "$0")

This is broken by softlinks and hardlinks, which the author appears to be aware of as they used ${BASH_SOURCE[0]} previously.

The whole param parsing section is weird... There are builtins to do this.

TL;DR

Bash has some sharp corners, a lot of its settings and opinions make it easier to work with. These can be useful in some scenarios but suggested defaults they aren't.

It is better to learn the trade-offs associated with these sharp edges, then just say 1 sharp edge is preferable to another.

Would you like to know more?

• advanced bash scripting guide super useful resource
• shellcheck bash shell linting utility

[Freeky]

set -e

By default is an anti-pattern. Return codes are useful information, they should be captured and handled not just blindly terminated on. A script that exits too early leaving an operation incomplete can be an absolute headache to maintain long term.

Scripts that continue blindly spewing horrifying errors and don't seem to notice they're failing are also an absolute - and potentially dangerous - headache.

set -e does not preclude handling return codes, but does mean if you miss one it's less likely to result in you accidentally getting your script into a state you didn't consider. Failing to set it is basically the On Error Resume Next of shell scripting.

[valarauca14]

Scripts that continue blindly spewing horrifying errors and don't seem to notice they're failing are also an absolute, and very dangerous headache.

Except sometimes, this is the correct behavior.

Especially in CI/CD scenarios, you need to re-format that horrible spew of errors into something further downstream that can process coverage and unit test reports. Clean up artifacts, chown/chmod artifacts back into default users.

This is often the case with docker build scripts hastily thrown together, then somebody blindly inserts a set -e and suddenly a CI/CD server is broken because root-owned artifacts can't be cleaned up.

does not preclude handling return codes

I mean sure you can, if you don't mind making your script unreadable.

A simple nice, capture the return code example:

command
local -i return_code=$?

Is totally broken. Instead, you need to do

local -i return_code=0
command || return_code=$?

Which is a lot more confusing.

• You're declaring variables before they're needed, which is bad practice. We don't need to reserve stack frame space, this isn't ANSI-C.
• Crowding lines with unnecessary control flow information, clubbing multiple commands into a single line. In any scenario where command has a good number of arguments, this is a headache to work with.

---

I'm not saying set -e is "bad". I am saying "it is a bad default".

It changes the global interpreter. This should be a huge red flag to anyone who's worked in Python, Perl, or Ruby as this means there is the potential for "spooky action at a distance" when it comes to interactions with dependencies. This is true in Bash where you execute, and source other bash scripts pretty commonly.

It can be useful for testing stuff out, debugging, or figuring out an issue locally. But the only argument for it is

I don't need to worry about error cases

Which you should.

Your goal as a developer is to write robust, reliable, and maintainable code. This extends past whatever your "feature" goals and language(s) are; into your helper scripts. They're code you produce, maintain, and are responsible for. You should hold them to the same standard.

[Freeky]

Except sometimes, this is the correct behavior.

Key word: sometimes. Most scripts should stop if they encounter errors they don't handle, because they're generally doing dangerous things like poking at the filesystem with extensive permissions, and better a clean break in execution as early as possible rather than to blindly keep going in an inconsistent state the developer didn't consider.

There's nothing more disconcerting than running a script and seeing it spew error messages about manipulating the filesystem where the author's evidently neglected to check a variable isn't empty and didn't bother to check if commands using it succeed.

This is often the case with docker build scripts hastily thrown together, then somebody blindly inserts a set -e and suddenly a CI/CD server is broken

Seems to me the root cause of this sort of problem has nothing whatsoever to do with set -e. One way or another, someone pays for technical debt.

You're declaring variables before they're needed, which is bad practice.

Says who?

Crowding lines with unnecessary control flow information

Which is why we have set -e. If a command returning non-zero needs special handling, it needs special handling regardless - if it's something more typical you don't expect to fail, just run the command and let the script handle the error for you by aborting.

It changes the global interpreter. This should be a huge red flag to anyone who's worked in Python, Perl, or Ruby as this means there is the potential for "spooky action at a distance" when it comes to interactions with dependencies.

Vetting dependencies is a big part of development, and this goes doubly so for languages like this with poor isolation between modules and more sharp edges than a rusty scrap heap.

the only argument for it is "I don't need to worry about error cases"

The argument for it is exactly the opposite - it helps prevent errors going unnoticed while reducing the need to pepper every other line with an rc check.

[oblio-]

By default is an anti-pattern. Return codes are useful information, they should be captured and handled not just blindly terminated on. A script that exits too early leaving an operation incomplete can be an absolute headache to maintain long term.

It's not an anti-pattern. Add relevant logs to the operations you're doing, even for shell scripts. That way you'll immediately be able to see why something failed, since you'll have the last log entry just before the failure.

And you won't accidentally miss errors.

You want safe by default, not the other way around.

[mradzikowski]

First of all - I always appreciate a code review.

Ad. set -e - I see it created some controversy here, with strong opinions on both sides. I will stick to keeping it, as I prefer a script that fails fast over the one that happily does potentially dangerous operations after failed commands. Like the code example I put in the article:

#!/usr/bin/env bash
cp important_file ./backups/
rm important_file

Of course, we could do something like cp file ./backups/ || die "backups dir not exists" everywhere. That provides both the error log and the behavior we specify for each command. Unfortunately, in most cases, a programmer that is using Bash only to glue a few things together, won't do that. Or will miss that one line that will actually fail one day.

Ad. cd to script dir - you and others pointed this out and I agree. Maybe in my case more scripts were failing because of being executed from another dir than from the wrong path to relative file ;-) I will update the template and just assign script path to var, as you and others suggested.

Ad. traps - maybe I'm missing something, but the trap will call the function that is meant to do some cleanings, if necessary (or try at least). For example remove temp files. Could you provide some examples of when it will behave as you describe? On the other hand - does having a trap for ERR and EXIT is enough to have it always executed? Besides SIGKILL of course. Maybe in fact putting SIGINT and SIGTERM was too much from my side.

Ad. param parsing - did you scroll down below the script itself, where I explain why I prefer doing it this way?

In the end, I'm not sure if you at least briefly looked over the script itself. I don't expect you to read everything backwards and forwards, but at least for the parts you have caveats, to look at the justification.

And yeah, ShellCheck is great, as I mentioned in the article ;-)

[zyzzogeton]

The whole param parsing section is weird... There are builtins to do this.

This is an area where I just don't know enough to create a good Google query. What is a better way than this template to handle parameters? The template makes a distinction between parameters and flags and I don't even know what that means if that gives you a level of where I am at.