2014-06-23 Oddmuse Migration

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.


if test "$1" == "-n"; then
    echo Not executing

if test -z "$1"; then
    F=`basename $0`
    echo Usage: $F [-n] DATADIR
    exit 1

if test ! -d "$1"; then
    echo You need to provide an existing directory
    exit 2

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 {} \;



😭 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 cases 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
alex@Margo:~$ a='hello world/test' && basename "$a"

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.

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:

if [[ $1 == '-n' ]]; then
    echo 'Not executing'

if [[ -z $1 ]]; then
    echo "Usage: ${0##*/} [-n] DATADIR" >&2
    exit 1

if [[ ! -d $1 ]]; then
    echo 'You need to provide an existing directory' >&2
    exit 2

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
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! :)

AlexSchroeder 2014-06-28 16:23 UTC

Another nice resource: 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

@Harald, actually Bash is not that bad. There were some wrong decisions like -r not being default for read, but these are minor. Do you think that all variables should be quoted automatically? And that programmers should “unquote” variables instead? I don’t think so.

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 ...

This comment was extracted to http://alexdaniel.org/2015-03-14_The_Downsides_of_Free_Food, check it out.

AlexDaniel 2014-07-02 08:09 UTC

Please make sure you contribute only your own work, or work licensed under the GNU Free Documentation License. Note: in order to facilitate peer review and fight vandalism, we will store your IP number for a number of days. See Privacy Policy for more information. See Info for text formatting rules. You can edit the comment page if you need to fix typos. You can subscribe to new comments by email without leaving a comment.

To save this page you must answer this question:

Please say HELLO.