Recent developments of Oddmuse forced me to write a little upgrade extension that helps users upgrade their installations. I’m unhappy with the entire thing.
Yes, I think upgrades are sometimes necessary. Ideally, users would never need to care. Additionally, users should never have to worry about installing all the upgrades they missed, in order. The usual way of doing this involves an *installer*. That’s tricky, too! What about permissions? Idiosyncratic installations? It’s messy.
What I did was write an extension that disables itself once it has run. This makes sure it has the same user, group, and permissions as the wiki itself.
And yet, I just discovered that something isn’t right with my permissions and ownership settings. I’ve had to run my *fix-data-dir-permissions* script again. The setup involves my account (alex) and the account that runs CGI scrips (www-data). Thus, the files and directories must belong to www-data and to the group alex. I am also part of group alex, which is why I will have write permissions for new files even though www-data creates them.
#!/bin/bash
if test "$1" == "-n"; then
echo Not executing
ECHO=echo
shift
fi
if test -z "$1"; then
F=`basename $0`
echo Usage: $F [-n] DATADIR
exit 1
fi
if test ! -d "$1"; then
echo You need to provide an existing directory
pwd
exit 2
fi
echo fixing directory permissions for `pwd`/$1
find $1 -type d -not -perm 2770 -exec $ECHO sudo chmod 2770 {} \;
echo fixing file permissions
find $1 -type f -not -perm 0660 -exec $ECHO sudo chmod 0660 {} \;
echo fixing ownership
find $1 \( -not -group alex -or -not -user www-data \) -exec $ECHO sudo chown www-data.alex {} \;
#Oddmuse
(Please contact me if you want to remove your comment.)
⁂
😭 It seems like bash is much harder than most people think. Weirdly, perl has a reputation of a hard-to-learn language, but bash is much, much worse, even though it takes an hour to learn it.
So, let me go through it line by line:
if test "$1" == "-n"; then
The use of test hardly makes any sense. You have bash shebang, and by using == you’re relying on bash completely (you don’t plan to have any compatibility), so why would you use test? Why not the bash `[[ ]]` syntax? Like that:
if [[ $1 == '-n' ]]; then
You have many quoting errors later... When in doubt, quote every variable everywhere. Extra quotes never hurt. However, you don’t have to quote left side of the expression in `[[ ]]` and there are some other **case**s when quotes are not needed, but it wont be too bad if you have excessive ones. Okay, next:
if test -z "$1"; then 1. to if [[ -z $1 ]]; then
Actually, it’s exactly the same thing as in the previous one.
F=`basename $0`
But this one is much worse. First of all, don’t use backticks, that’s an old way. Use **$()** ! It is much cleaner, you will not confuse these with regular quotes, but most importantly you can nest it! That was just a style error, but look! No quotes around `$0`. And yes, you need them, otherwise it will break if your path has whitespace.
You can see the difference here:
alex@Margo:~$ a='hello world/test' && basename $a hello alex@Margo:~$ a='hello world/test' && basename "$a" test
As an exercise, guess what would happen if `a='* world/test'`
Basically you’ve got the same quoting error on the next line. Sooo, I changed both lines like this:
echo "Usage: $(basename -- "$0") [-n] DATADIR" >&2
Some people would instantly scream “HEY! These quotes are wrong! Your `$0` variable is not quoted, because first `"` matches with second `"` and third matches with fourth, leaving `$0` in the middle unquoted”. Well, that’s not the case. At least not in bash. Bash is smart enough to treat this situation correctly, quotes inside `$()` will not match with quotes outside of it. This actually makes sense, because how else would you quote parameters if it worked like some people expect?
Now, when the lesson is learned, we can forget basename and use parameter expansion instead.
echo "Usage: ${0##*/} [-n] DATADIR" >&2 # Wheee!
Well, that’s it, basically. I have also added >&2 to print into stderr instead of stdout, quoted other unquoted variables, added single quotes around plain strings (this is just an indication for plain strings, it gives a nice syntax highlighting too!), added `--` in case of weird file names with leading slashes (this is covered in Pitfall #3), changed pwd call to $PWD. Many of these are minor style mistakes, but I believe that some people might commit suicide after seeing bash scripts like these again and again 😄
If you have an hour or so, read through this list http://mywiki.wooledge.org/BashPitfalls , it helps so much.
http://mywiki.wooledge.org/BashPitfalls
I hope you don’t mind these corrections, I just feel like if I don’t explain such issues people will continue doing mistakes.
Here is the whole script:
#!/bin/bash
ECHO=''
if [[ $1 == '-n' ]]; then
echo 'Not executing'
ECHO=echo
shift
fi
if [[ -z $1 ]]; then
echo "Usage: ${0##*/} [-n] DATADIR" >&2
exit 1
fi
if [[ ! -d $1 ]]; then
echo 'You need to provide an existing directory' >&2
exit 2
fi
echo "fixing directory permissions for $PWD/$1"
find -- "$1" -type d -not -perm 2770 -exec $ECHO sudo chmod 2770 {} \;
echo 'fixing file permissions'
find -- "$1" -type f -not -perm 0660 -exec $ECHO sudo chmod 0660 {} \;
echo 'fixing ownership'
find -- "$1" \( -not -group alex -or -not -user www-data \) -exec $ECHO sudo chown www-data.alex {} \;
Oh, and `$ECHO` is unquoted because unquoted variables indicate ✨MAGIC✨ and that’s what you’re doing with $ECHO 😄
– AlexDaniel 2014-06-28 05:13 UTC
---
Wow, this is awesome! Thank you very much for all the explanations.
– AlexSchroeder 2014-06-28 08:28 UTC
---
Owww, I missed one. If you don’t initialize ECHO then it will pass any variable from the environment. For example if you run your script like this:
ECHO=rm ./thatscript mydir #or export ECHO=rm ./thatscript mydir
You might face interesting results (with ECHO being passed to -exec, ouch!). That’s why it is a good idea to initialize variables in bash
– AlexDaniel 2014-06-28 13:48 UTC
---
Haha, this is great. Thank you! 😄
– Alex Schroeder 2014-06-28 16:23 UTC
---
Another nice resource: https://google-styleguide.googlecode.com/svn/trunk/shell.xml
https://google-styleguide.googlecode.com/svn/trunk/shell.xml
Also make sure to treat variables that may have spaces in them (looking at directory paths and file names) correctly or you may end up with broken stuff.
Oh, and look at getopts.
Your first sentence is true though: Instead of bash, it makes sense to look at and use a more modern scripting language, dropping 20-30 years of baggage along the way.
– Harald 2014-06-29 09:32 UTC
---
Also, what about variables with spaces? I have fixed all these issues. Can you spot something that I missed?
Yeah, getopts is the way to go. I just felt like it is not worth the hassle to introduce getopts just for one tiny parameter.
Now, about that google-styleguide... I don’t suggest to read it, it is plain ...
http://alexdaniel.org/2015-03-14_The_Downsides_of_Free_Food
– AlexDaniel 2014-07-02 08:09 UTC