projg2 / installkernel-gentoo

Gentoo fork of installkernel from debianutils
GNU General Public License v2.0
19 stars 7 forks source link

On Bash's `set -u` usage #25

Closed kuraga closed 8 months ago

kuraga commented 8 months ago

There is an option to use -u mode of running scripts:

#!/bin/bash
set -u

https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/utilities/V3_chap02.html#set:

-u Treat unset variables as an error when substituting. When the shell tries to expand an unset parameter other than the '@' and '*' special parameters, it shall write a message to standard error and the expansion shall fail with the consequences specified in Consequences of Shell Errors.

This helps to avoid catastrophic consequences in commands like rm -rf "path/to/${arg}". Also IMHO it could partially help in the KDE Themes incident.

And, well, it's just a good practice? I've been surprised it's not by default.

@AndrewAmmerlaan , I'd very appreciate your opinion on this. And, were there some cases in Gentoo, generally?

Thanks!

Nowa-Ammerlaan commented 8 months ago

The thing with 'set -u' is that this works on all variables. Whereas many scripts here work with variables that may or may not be set. I'd prefer to use :? instead where it is appropriate, so we have finer control on which variables must be set and which ones are optional. FWIW Shellcheck reports it when you try to rm -rf with some variable expansion that may be unset.

kuraga commented 8 months ago

Got it.

Whereas many scripts here work with variables that may or may not be set.

${VARIABLE:-default}?

Nowa-Ammerlaan commented 8 months ago

Got it.

Whereas many scripts here work with variables that may or may not be set.

${VARIABLE:-default}?

Yes this is something we do in some places. We also have some [[ -z ... ]] and [[ -n .... ]] for variables that are only used if set. For example, if INSTALLKERNEL_INTRD_GENERATOR is set to something other than dracut, the dracut plugin is skipped. Here we cannot use :- or set -u since the variable not being set is a valid setting.

kuraga commented 8 months ago

Ok, thanks!