6
\$\begingroup\$

I am currently trying to learn bash scripting and I've made a script that backs up or restores my home folder. My main question is whether my code is readable or very messy. I would love to learn to write my scripts understandable to others and also optimally. This script isn't made for sharing right now. Any tips on how I can make it better in any way? (Of course, the answer is yes, but I'm looking for specifics.) This is my first real script, so please don't be harsh.

#!/bin/bash

# Find where device is mounted and print so the user can check if correct.
BACKUP_DEV_UUID=#ENTER UUID HERE#
BACKUP_DEV_NODE=$(sudo blkid | grep $BACKUP_DEV_UUID | cut -f1 -d':')
if [[ ! $BACKUP_DEV_NODE =~ /dev/* ]]; then
    echo "Error, device not connected"
    exit 0
fi
MOUNT_POINT=$(mount | grep $BACKUP_DEV_NODE | cut -f3 -d' ')
echo "Mount point is: $MOUNT_POINT"

# Determine if you want to backup, restore of quit. Then fix full path.
echo -e "Input 1 to backup, 2 to restore, anything else to quit"
read -p "Enter choice: " -n1 fc
echo -e ""
case $fc in
    1) echo "Backup chosen"
    FULL_PATH=$MOUNT_POINT/$(hostname)_backup/home/$USER
    if [[ ! -d $FULL_PATH ]]; then
        echo "Backup path doesn't exist, creating"
        mkdir -p $FULL_PATH
    fi  
    ;;

    2) echo "Restore chosen"
    # Choose hostname and username folders
    echo "Select appropriate hostname folder"
    select hn in $MOUNT_POINT/*_backup;
    do
        echo "Picked hostname folder: $HOSTNAME_VAR"
        echo "Select appropriate username folder"
        select un in $HOSTNAME_VAR/home/*;
            do
                echo "Picked username folder: $USERNAME_VAR"
                FULL_PATH=$USERNAME_VAR
                break
            done
        break
    done    
    ;;

    *) echo "Exiting";
    exit 0
    ;;
esac

# Print dest/source and ask user if accepted.
case $fc in
    1) echo "Doing home folder backup"
    echo "Backup to path: $FULL_PATH"
    ;;

    2) echo "Doing home folder restore"
    echo "Restore from path $FULL_PATH"
    ;;
esac
read -r -n1 -p "Continue? Press y/Y to start backup/restore"
echo 
if [[ ! $REPLY =~ ^[Yy]$ ]]; then
    echo "Aborting"
    exit 0
else
    echo "Continuing"
fi

# Actual backup/restore.
case $fc in
    1) rsync -aAXv ~/* $FULL_PATH --delete
    echo $(date) >> $FULL_PATH/homefolder_backup.log
    ;;

    2) rsync -aAXv $FULL_PATH ~/*
    ;;
esac
echo "Syncing"
sync
echo "Done"
\$\endgroup\$
4
\$\begingroup\$

This is very nice for a beginner!


Instead of matching against files in the filesystem as a glob like this:

if [[ ! $BACKUP_DEV_NODE =~ /dev/* ]]; then

It would be better to use the -e flag to check if the path exists or not:

if [[ ! -e $BACKUP_DEV_NODE ]]; then

It's better to avoid any of the flags of echo, because they don't work consistently in all systems.

In these examples you don't need any flags:

echo -e "Input 1 to backup, 2 to restore, anything else to quit"
echo -e ""

You could simplify these as:

echo "Input 1 to backup, 2 to restore, anything else to quit"
echo

If you find yourself needing any of echo's flags, look into printf instead.


In here:

select hn in $MOUNT_POINT/*_backup;
do
    echo "Picked hostname folder: $HOSTNAME_VAR"
    echo "Select appropriate username folder"
    select un in $HOSTNAME_VAR/home/*;
        do
            echo "Picked username folder: $USERNAME_VAR"
            FULL_PATH=$USERNAME_VAR
            break
        done
    break
done

You're not using hn. Another thing, the do...done of the inner select is over-indented, which is not consistent with the rest of the code.


The default exit code is that of the last operation. So unless you want to forcefully exit with 0 after a failed operation, you shouldn't need exit 0, you can write simply exit.


Instead of this:

echo $(date) >> $FULL_PATH/homefolder_backup.log

You could simply redirect the output of date itself:

date >> $FULL_PATH/homefolder_backup.log

The double-quotes here are unnecessary:

echo "Syncing"
echo "Done"
\$\endgroup\$
2
\$\begingroup\$

Assuming this will at some point be migrated to codereview.stackexchange.com…

  1. All-caps names should conventionally be reserved for exported environment variables intended for subprocesses.
  2. Quote your expansions, or the shell will wordsplit them for you. For example, grep $BACKUP_DEV_UUID is different from grep "$BACKUP_DEV_UUID".
  3. Combine "grep" and "cut" with awk. awk -F: "/${BACKUP_DEV_UUID}/"'{print $1}'
  4. Your regex probably doesn't do what you intend. Use lightweight matching expressions before resorting to regex. if [[ $BACKUP_DEV_NODE != /dev/* ]]…. (What you have would match "/dev" and "/dev////", but not "/dev/foo".)
  5. Bash has a special syntax for interpreting backslash escapes, which makes echo -e in my opinion the least useful of your three options: (echo -e, printf, and $'' string handling).
  6. Consider the environment you're operating in. There's a very good chance HOSTNAME is already exported into it, so you don't need the extra subprocess expansion for $(hostname).
  7. if [[ $REPLY != [yY]* ]] or shopt -s nocasematch; if [[ $REPLY != y* ]]…
  8. You're using $(date) as a command. That means the shell will expand it, then attempt to execute it. What happens when you execute Sun Sep 28 12:24:24 EDT 2014? You're probably be putting an empty string into your log file and printing an error message to stderr. I think you mean date >> "$FULL_PATH/homefolder_backup.log".
  9. Consider setting LC_ALL=C at the top of this to avoid having its behavior change due to particular locales.
\$\endgroup\$
  • \$\begingroup\$ Thank you for all the suggestions! :) 3. What does the {} do different from () in this case? or the absence of {} around backup_dev_uuid? 8. I posted the code before I noticed the missing echo before '$(date)'. But this is fixed now. \$\endgroup\$ – Maximus Sep 28 '14 at 16:35
  • \$\begingroup\$ Technically, the {} helps the parser delimit the name you want to expand. Here it doesn't make any difference except readability, because slash characters can't be part of a name anyway, so awk -F: "/$BACKUP_DEV_UUID/"'{print $1}' would do as well. \$\endgroup\$ – kojiro Sep 28 '14 at 16:38
  • \$\begingroup\$ For 8, don't use echo, just use date. \$\endgroup\$ – kojiro Sep 28 '14 at 16:38
  • \$\begingroup\$ $(mount | awk "/${backup_dev_node}/"'{print $3}') just gives me errors. Any idea why? awk: line 1: syntax error at or near { \$\endgroup\$ – Maximus Sep 28 '14 at 17:10
  • \$\begingroup\$ If $backup_dev_node is expanding to something interpretable as a regular expression, that could confuse AWK. \$\endgroup\$ – kojiro Sep 28 '14 at 17:14
1
\$\begingroup\$

Use

#!/usr/bin/env bash

instead of

#!/bin/bash

learn more

From wiki:

Note that it is possible to specify the interpreter without using env, by giving the full path of the python interpreter. A problem with that approach is that on different computer systems, the exact path may be different. By instead using env as in the example, the interpreter is searched for and located at the time the script is run. This makes the script more portable, but also increases the risk that the wrong interpreter is selected because it searches for a match in every directory on the executable search path. It also suffers from the same problem in that the path to the env binary may also be different on a per-machine basis.

User can config their enviroment as he likes, replace default path and so on. That's why using env to define what you should actually use as bash is good practice (in case env is located in default location).

\$\endgroup\$
  • \$\begingroup\$ If I understand correctly this will ensure that bash will be run from my filesystem rather than from shell. Is this correct? Or are there more pros? \$\endgroup\$ – Maximus Sep 28 '14 at 18:52
  • \$\begingroup\$ Check wiki's notes below examples. \$\endgroup\$ – outoftime Sep 28 '14 at 19:01

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.