-1

I use the following code which is part of this script that I use to update my WordPress websites:

#!/bin/bash
drt="/var/www/html"
for dir in ${drt}/*/; do
    if pushd "$dir"; then
        wp plugin update --all --allow-root
        wp core update --allow-root
        wp language core update --allow-root
        wp theme update --all --allow-root
    popd
    fi
done

I learned of the specific pushd-popd pattern used in this code when I searched for a way to update all my WordPress instances in one go.

It's unclear to me why this code includes an if-fi segment.

My question

Can I change the syntax somehow so that I'll have basically the same pattern but without an if-fi segment?

For example, instead this:

if pushd "$dir";
popd
    commands
fi

I'll have something like this pseudocode:

pushd "$dir";
    commands
popd

Why I ask this

I can imagine how we would tell the computer something like this, without an if-fi statement (pseudocode):

for dir in ${drt}/*; do pushd "$drt"; then
    commands
popd

Notes

  1. You might want to include a different approach in your answer (i.e without pushd-popd at all).

  2. A related question.

6

In addition to what @Olorin wrote I think there might be a couple misunderstandings here. First, for y in ${x}/*; do pushd "$y"; then results in

bash: syntax error near unexpected token `then'


Second, the indentation may be misleading you with regards to what is actually happening. Take this properly formatted version of the original code:

for y in ${x}/*/
do
    if pushd "$y"
    then
        command1
        command2
        popd
    fi
done

In other words, all of command1, command2 and popd run only if the initial pushd succeeded. If you instead wrote

for y in ${x}/*/
do
    pushd "$y"
    command1
    command2
    popd
done

and there was no errexit guard in place, a failing pushd or popd would not affect the rest of the script. This could result in running command1 and command2 in the wrong directory and then possibly popping back to another directory unrelated to this code. This could have disastrous consequences.


Finally I would argue that pushd + commands + popd is an anti-pattern because it adds more context (and therefore cognitive overhead and risk) to a language where complex context is already a massive problem. The most common way to work around this is to pass the path (ideally absolute) to the commands, like this:

for y in "$x"/*/
do
    command1 "$y"
    command2 "$y"
done
1

What happens if the pushd fails? If the commands are to be run in those directories, it is obvious that there should be a check to see if pushd succeeded before proceeding with the commands.

  • Why not just give an error like pushd cannot access the collection (directory) and be done with it? – user9303970 Apr 19 '18 at 1:16
  • The error is given, but... Be done with what? The pushd command? That iteration of the loop? The loop itself? The script? The OS session? – Olorin Apr 19 '18 at 1:24
  • Be done with both the loop and the pushd command. – user9303970 Apr 19 '18 at 15:39
  • If that's the case, you can just use pushd "$dir" || break. – Olorin Apr 20 '18 at 1:08
  • No need to update after edit with ` "$dir" || break`? – user9303970 Apr 23 '18 at 3:09
1

Using an if is a reasonable protection against a failure to change directory.

This code will execute the commands in the parent directory if the directory is not executable (no x permision).

#!/bin/bash
drt="/var/www/html"

for     dir in "${drt}"/*/
do      pushd "$dir"
        pwd
        popd
done

Build a couple of directories, change owner and permissions :

$ mkdir -p /var/www/html/{one,two}
$ sudo chown user:user /var/www/html/{one,two}
$ sudo chmod o-x /var/www/html/two
$ ./script
/var/www/html/one ~/temp
/var/www/html/one
~/temp
./script: line 5: pushd: ./var/www/html/two/: Permission denied
~/temp
./script: line 7: popd: directory stack empty

The command pushd emited an error, but the command pwd was executed in the ~/temp directory (note the value of ~/temp printed just after the error). That is a clear risk of doing the wrong thing. Compare with this script:

#!/bin/bash
    drt="./var/www/html"

    for     dir in "${drt}"/*/
    do      if     pushd "$dir" 2>/dev/null
            then
                   pwd
                   popd
            fi
    done

The new script executed:

$ ./script
/var/www/html/one ~/temp
/var/www/html/one
~/temp

Or even better:

#!/bin/bash
drt="./var/www/html"

for     dir in "${drt}"/*/
do      if     pushd "$dir" 2>/dev/null
        then
               pwd
               popd
        else
                echo "Failed to change to dir=$dir" >&2
                exit 7
        fi
done

Which, on execution, will print:

$ ./script
/var/www/html/one ~/temp
/var/www/html/one
~/temp
Failed to change to dir=/var/www/html/two/
1

If you are working with a */ glob, the shell might do the permission check for you if you use */. instead.

$ mkdir x y z
$ chmod -x y
$ echo */
x/ y/ z/
$ echo */.
x/. z/.

That way it should be a lot less likely for pushd to fail. However this could also be considered bad style, as that code does not really make clear that */. has such an intention. With if the intention is obvious: make sure it succeeds, or else.

Always checking every single command for errors would be the right thing to do in general. So do it if it can be done easily and without convoluting your code too much.

However it is, unfortunately, also normal to not check every single command for errors and just hope/trust things will work as intended most of the time. A lot of shell scripts are meant to be simple helpers and quick hacks only.

Adding a check for every possiblity AND write code to handle each possible error in a sane manner, would quickly become convoluted and unreadable. Code readability is important, too.

popd can fail in some (obscure) cases too. Someone renamed the directory? Too bad. Nobody checks for these things because it's not worth it. shrugs

If you do not modify any variables inside that loop of yours, you could do directory your switches in subshells as opposed to pushd/popd. Each subshell has its own working directory while the parent shell keeps theirs. But if you recurse deeply it will give you a subprocess hell instead of a stack of subdirs to keep track of.

Working with absolute paths throughout is sometimes also an option.

1
+100

With gnu-find, you may do:

find ${drt} -maxdepth 1 -type d -execdir command1 ";" -execdir command2 ";"

Note that not every find implementation has an -execdir option.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.