I want to backup a single SQLite database daily up to 30 days back, but I also want to keep at least 2 backups at all times (i.e. if there have been no backups in the last 30 days because the database didn't change, I don't want to delete old backups).
I came up with this simple script that is supposed to run as a daily cronjob:
#!/bin/bash
BACKUP_FILE="/path/to/db.sqlite3"
BACKUP_DIR="$HOME/backups"
today=`date "+%Y-%m-%d"`
# Less than 31 days old, i.e. 30 days or younger
if find "$BACKUP_FILE" -type f -mtime -31 | grep -q .
then
find "$BACKUP_DIR" -type f -mtime +30 -exec rm {} \;
fi
last_backup=$(ls -t "$BACKUP_DIR" | head -1)
if [ -n "$last_backup" ] && diff "$BACKUP_FILE" "$BACKUP_DIR/$(ls -t "$BACKUP_DIR" | head -1)" >/dev/null
then :
else
cp "$BACKUP_FILE" "$BACKUP_DIR/$today.sqlite3"
fi
I'm not sure if using a nop in the second then
clause makes sense, but it seemed cleaner to me than wrapping the condition in a test
and checking $?
.
The database is used by <20 people and not changed very often, but I'm not sure if I should lock the database anyways - and I'm also not sure how to lock it from a shell script.
Since this is the first bash script I've ever written for serious use, I'd appreciate any feedback on how it could be improved.
1 Answer 1
Don't use the obsolete syntax `...`
Use the modern syntax $(...)
:
today=$(date "+%Y-%m-%d")
Simplify file deletion
Instead of deleting files with -exec rm {} \;
, a simpler way is using -delete
:
find "$BACKUP_DIR" -type f -mtime +30 -delete
Don't use the obsolete syntax head -N
head -1
is obsolete syntax. Use head -n 1
instead.
Don't repeat yourself
On these lines, $(ls -t "$BACKUP_DIR" | head -1)
is repeated:
last_backup=$(ls -t "$BACKUP_DIR" | head -1) if [ -n "$last_backup" ] && diff "$BACKUP_FILE" "$BACKUP_DIR/$(ls -t "$BACKUP_DIR" | head -1)" >/dev/null
The second use should be replaced with $last_backup
.
Compare files using cmp
Instead of diff
, it's more efficient to compare files using cmp
:
if [ -n "$last_backup" ] && cmp -s "$BACKUP_FILE" "$BACKUP_DIR/$last_backup"
Avoid empty statements
Instead of the empty then
clause of the final condition,
it's better to invert it (using DeMorgan's law):
if [ ! "$last_backup" ] || ! cmp -s "$BACKUP_FILE" "$BACKUP_DIR/$last_backup"
then
cp "$BACKUP_FILE" "$BACKUP_DIR/$today.sqlite3"
fi
Delay operations until really needed
The today
variable is only used once at the end,
when certain conditions are fulfilled.
It would be better to create it right before it's needed.
-
1\$\begingroup\$ Maybe also point out mywiki.wooledge.org/ParsingLs though there isn't a really elegant fix for that. \$\endgroup\$tripleee– tripleee2017年02月09日 13:06:32 +00:00Commented Feb 9, 2017 at 13:06