Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

As a follow up to a previous question, linked here, I have revised the code and developed what I believe to be a better solution.

In summary, the script should backup all files in a particular directory, and assign them to a particular zip file based on their modification dates. This should only occur for files modified yesterday and older, and skip files with the ".zip" extension.

Please let me know if their are any improvements that you would make? From testing the code, it has been working with no issues:

#!/bin/bash

yestdayend=$(date --date="yesterday" +"%Y-%m-%d 23:59:59")

path=/path/to/dir
filelist=$(find $path -maxdepth 1 ! \( -name "*.zip" \) -type f ! -newermt "$yestdayend")

for file in $filelist
do 
        moddate=$(stat -c %y $file | cut -d " " -f 1)
        if zip -rv $path/"logbackup-"$moddate.zip $file; then
           rm $file
        fi
done
share|improve this question

1 Answer 1

up vote 2 down vote accepted

Saving the output of find in a variable and looping over it is not a good idea in general: filenames with white-space (space, tab, newline) will be split, so the loop will not work on them.

It would be a little bit better to use a while loop instead:

find ... | while read file
do 
        # ...
done

But this is still not great, as it won't protect you from newlines. But for your use case, it might be good enough. For a more robust solution, see this other post.


The \( ... \) is pointless here:

find $path -maxdepth 1 ! \( -name "*.zip" \) -type f ! -newermt "$yestdayend"

This is the same, but shorter and simpler:

find $path -maxdepth 1 ! -name "*.zip" -type f ! -newermt "$yestdayend"

It's a bit odd to quote "logbackup-" here:

if zip -rv $path/"logbackup-"$moddate.zip $file; then

Literal strings like that don't need quoting in Bash. On the other hand, it would be good to quote $path:

if zip -rv "$path"/logbackup-$moddate.zip $file; then
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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