A shell script to mount a disk image file

Question

I'm trying to create a bash shell script which mounts a disk image file. Not only that but checks to see if the disk image already exists. Is there anyway I could improve my script? Currently my script looks like this:

#!/bin/bash

MOUNTPOINT="/myfilesystem"

if [ ! -d "${MOUNTPOINT}" ]; then
    if [ -e "${MOUNTPOINT}" ]; then
        echo "Mountpoint exists, but isn't a directory..."
        exit 1
    else
        sudo mkdir -p "${MOUNTPOINT}"
    fi
fi

if [ "$(echo ${MOUNTPOINT}/*)" != "${MOUNTPOINT}/*" ]; then
    echo "Mountpoint is not empty!"
    exit 1
fi

if mountpoint -q "${MOUNTPOINT}"; then
    echo "Already mounted..."
    exit 0
fi

Answer 1

The first conditional group can be reorganized to make it flatter and simpler:

if [ ! -e "${MOUNTPOINT}" ]; then
    sudo mkdir -p "${MOUNTPOINT}"
elif [ -d "${MOUNTPOINT}" ]; then
    echo "Mountpoint exists, but isn't a directory..."
    exit 1
fi

The exit 0 is pointless in the last condition, and as such, you could replace it with a one-liner using &&:

mountpoint -q "${MOUNTPOINT}" && echo "Already mounted..."

Answer 2

• Error messages should be printed on standard error, like `echo "Chocolate missing!" >&2
• Internal variables should be lowercase, like mount_point. Don't worry if it ends up being the same string as a command like mountpoint; Bash handles that just fine (and your IDE should highlight them to show the difference).
• The idiomatic Bash shebang line is #!/usr/bin/env bash (disclaimer: my answer)
• sudo is generally discouraged within scripts, for at least two reasons:
  1. The target system may not have sudo. This is uncommon, but some use su instead.
  2. It makes it harder to automate the use of the script, since sudo may prompt for a password.
• It is generally expected of a shell script that it will not have unintended side effects. Your script will create the mount point directory if it doesn't exist.

• Instead of hard coding your mount point you can easily make the script … scriptable:

  for mount_point
  do
      [the contents of your script]
  done
  

  this will loop through all the arguments you provide ("$@")

• I personally prefer moving the then and do keywords to a separate line, to avoid having to use ; in my scripts.