Return to Answer

• 206
• 1
• 5

• 206
• 1
• 5

There are a coupe of bad practices here. First, as a general rule, you should avoid using CAPS for variable names in shell scripts. By convention, global environment variables are capitalized so making your own, local variables upper case can lead to naming collisions and very hard to debug bugs. It is much better to use lower case variable names for internal variables.

Next, you should always quote your variables. In this case, since you control the values, it is less of a danger tan it could be, but you still should quote them. See this excellent Q&A over at Unix & Linux: https://unix.stackexchange.com/q/171346/22222.

In your specific case, this find command is fragile:

find $BACKUP_DIR* -mtime $KEEP -exec rm {} \; >> $MSG 2>> $MSG

You haven't told us what this command is supposed to do, so I a guessing that you want to search through all directories who name starts with the value of $BACKUP_DIR. If that's really what you mean, you might want to change the name of the variable to backup_dir_prefx or something to make that clear.

If instead you just want to search in the directory $BACKUP_DIR, then remove the *. The shell will expand the wildcard (*) before calling find which means that the unquoted $BACKUP_DIR* will match backupNoThisDirectoryEver or anything else starting with the value of $BACKUP_DIR. Plus, this will fail if you ever change the variable to a value containing a space or wildcard characters.

Next, if you are running this on Linux or more generally using GNU find, you can use the -delete option. If not, you can at least end the command with + instead of \; to delete multiple files with the same command for efficiency. From man find:

 -exec command {} +
 This variant of the -exec action runs the specified command on
 the selected files, but the command line is built by appending
 each selected file name at the end; the total number of invoca‐
 tions of the command will be much less than the number of
 matched files. [...]

Finally, since you're using bash, you can use &> to redirect both stderr and stdout. Putting all of this together gives you:

find "$backup_dir"* -mtime "$keep" -delete &>> "$MSG"

Or, if you don't want to look in other directories:

find "$backup_dir" -mtime "$keep" -delete &>> "$MSG"

And if this is not using GNU find:

find "$backup_dir" -mtime "$keep" -exec rm -r {} + &>> "$MSG"

Also note that this command will also return "$backup_dir" as a result:

$ find backup/
backup/
backup/file.txt

If the backup directory itself matches the -mtime you set, then your find will also delete the entire directory. You could avoid that with -mindepth 1 if you are using GNU find.