4
\$\begingroup\$

I inherited a script at work the other day. I know very little about the command line in general but I'm not entirely new to programming. I am using this as an opportunity to learn... Very little of the original code is left (so if it's all wrong, it's my fault now). I was asked to modify it retain sub-directories so I chose to use tar.

The directories being archived are huge; maybe not crazy huge but definitely big. It should be ran often enough where there aren't too many files to process. Hopefully. I'm not sure how concerned I should be about that yet. It can be assumed that more than once instance of this script will never be running at the same time.

function archive() {
   log_dir=$1       # /home/u123/initial_files/SessLogs
   age=$2           # age of the files to process
   archive=$3       # base of the archive directory
   exclude=$4       # exclude_list of directories/files

   base_dir=${log_dir%/*}  # /home/u123/initial_files
   ftype=${log_dir##*/}    # SessLogs

   date_dir=$(make_date_string $age)

   # make the new folder in the archive with the date
   arch_dir=$archive/$ftype/$date_dir
   [ ! -d $archive/$ftype ] && ( mkdir $archive/$ftype )
   [ ! -d $arch_dir ] && ( mkdir $arch_dir )

   outfile=$arch_dir/archivelog.txt
   printf "$(date)""\n\n" >>$outfile

   printf "%s\n" "processing $ftype files..." >>$outfile
   printf "%s\n" "  - type    = $ftype"       >>$outfile
   printf "%s\n" "  - age     = $age days"    >>$outfile
   printf "%s\n" "  - log_dir = $log_dir"     >>$outfile
   printf "%s\n" "  - archive = $archive"     >>$outfile
   printf "%s\n" "  - exclude = $exclude"     >>$outfile
   printf "\n">>$outfile

   # test for valid age
   if [ $age -lt 1 ]; then
      printf "\n%s\n" "age parameter is invalid" >>$outfile
      return 0
   # test for existence of log file
   elif [ ! -d $log_dir ]; then
      printf "\n%s\n" "log directory cannot be found $log_dir" >>$outfile
      return 0
   # test for existence of exclude list
   elif [ ! -f $exclude ]; then
      printf "\n%s\n" "exclude list cannot be found $exclude" >>$outfile
      return 0
   fi

   # i hate to run the full find twice since these are very large directories... idk.
   files=$( find "$log_dir" -type f -mtime +"$age" | grep -v -f "$exclude" | wc -l )

   # no files to move...
   if [[ "$files" -eq "0" ]] ; then
      #printf "find ""$log_dir"" -type f -mtime +"$age" | grep -v -f "$exclude" | wc -l" >>$outfile
      printf \n"%s\n" "there are no files to update for $log_dir" >>$outfile
      return 0
   fi

   # save this for after tar is done
   # get directories that have old files in them and give me a unique list sorted in reverse
   cleanup=( $( find "$log_dir" -type f -mtime +"$age" | grep -v -f "$exclude" | sed 's/\(.*\)\/.*/\1/' | sort -ur ) )

   # shove the files in a .tar into the archive directory for this date
   find "$log_dir" -type f -mtime +"$age" | grep -v -f "$exclude" | sort -r \
        | tar vciPhf "$arch_dir"/archive.tar --remove-files --same-owner --atime-preserve --files-from - 1>&2>>$outfile

   if [ "$?" != "0" ]; then
      printf "\n%s\n" "process stopped for $ftype because of errors with .tar" >> $outfile
      return 0
   fi

   # unzip the tar file in the archive directory
   sc=$( grep -o "/" <<< "$log_dir" | wc -l )
   tar xvp -C "$arch_dir" -f "$arch_dir"/archive.tar --strip-components="$sc" 1>&2>>$outfile

   printf "\n\n" >>$outfile

   # if no error: rm "$arch_dir"/archive.tar
   if [ "$?" = "0" ]; then
      for dir in "${cleanup[@]}"; do
         ct=$( find $dir | wc -l )
         if [ "$ct" -eq 1 ]; then
            printf "rmdir ""$dir""\n" >>$outfile
            rmdir "$dir"
         fi
      done

      printf "\n\n%s\n" "archive successful" >>$outfile
      rm "$arch_dir"/archive.tar
      return 1
   fi 

   return 0
}

# this is entirely from the original code 
function make_date_string() {
   local -ix age=$1
   calc_time=$(expr $age \* 86400)
   date_calc=`eval "perl -e 'print scalar localtime( time - $calc_time ) . \"\n\";'"`
   final_date=`date +%Y-%m-%d --date="$date_calc"`
   echo $ftype"_"$final_date  # creates date string 
}

# a settings file will be read in... it's just this way for testing right now
archive '/home/u123/initial_files/SrcFiles' 7 '/home/u123/ArchiveFiles' '/home/u123/exclude_list'
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review. Please indicate what exactly you did inherit (it is not subject to review per CR charter), and what is your contribution to be reviewed. \$\endgroup\$ – vnp Oct 25 '14 at 23:29
  • \$\begingroup\$ I pretty much stated that (I thought clearly) in the post. The only thing left over is the make_date_string function. \$\endgroup\$ – gloomy.penguin Oct 25 '14 at 23:31
  • \$\begingroup\$ the original file was the date thing and a parameter form of (not verbatim) find ./stuff -type f -mtime +N -exec mv {} \; It needed to use an exclude list and keep sub-directory structure. And logging, which I'm doing now. \$\endgroup\$ – gloomy.penguin Oct 25 '14 at 23:37
  • 1
    \$\begingroup\$ @vnp I disagree. OP is clearly responsible for maintaining all of the code. \$\endgroup\$ – RubberDuck Oct 26 '14 at 2:36
  • \$\begingroup\$ I mean... I keep debating on whether I should post a portion of the original to prove everything is mine but that date part. But I really don't feel comfortable posting someone else's work for any reason. With comments and whitespace removed, including the date stuff, Egyptian brackets - the original function was 12 lines long and used global variables. \$\endgroup\$ – gloomy.penguin Oct 26 '14 at 5:30
2
\$\begingroup\$

You can simplify this:

arch_dir=$archive/$ftype/$date_dir
[ ! -d $archive/$ftype ] && ( mkdir $archive/$ftype )
[ ! -d $arch_dir ] && ( mkdir $arch_dir )

to this:

[ -d $arch_dir ] || mkdir -p $arch_dir

That is:

  • The -p flag of mkdir will make it create all intermediary directories as necessary
  • No need to put (...) around the mkdir
  • Using ... || instead of ! ... && is shorter, and perhaps even more natural too

printf "$(date)""\n\n" >>$outfile

First of all, unless you have a good reason, don't use a $(...) subshell to capture the output of a command just to print it. Use the output directly:

date >>$outfile

Secondly, if a simple echo without parameters is enough, I would use that instead of a printf. If you need any of the flags of echo, then it's better to use printf, as it's more portable. For printing a blank line, I'd use echo.


Redirecting many lines to $outfile like this feels a bit repetitive:

printf "%s\n" "processing $ftype files..." >>$outfile
printf "%s\n" "  - type    = $ftype"       >>$outfile
printf "%s\n" "  - age     = $age days"    >>$outfile
printf "%s\n" "  - log_dir = $log_dir"     >>$outfile
printf "%s\n" "  - archive = $archive"     >>$outfile
printf "%s\n" "  - exclude = $exclude"     >>$outfile
printf "\n">>$outfile

A better way would be:

{
    printf "%s\n" "processing $ftype files..." 
    printf "%s\n" "  - type    = $ftype"       
    printf "%s\n" "  - age     = $age days"    
    printf "%s\n" "  - log_dir = $log_dir"     
    printf "%s\n" "  - archive = $archive"     
    printf "%s\n" "  - exclude = $exclude"     
    printf "\n"
} >> $outfile

As mentioned earlier, an even better way would be rewriting these with simple echo, for example:

echo "processing $ftype files..." 

Finally, since you have many lines appending to $outfile, it might be best to create a helper function for it.


if [[ "$files" -eq "0" ]] ; then

When using [[ ... ]], you don't need to quote variables on the left side. You also don't need to quote barewords like "0". So this is enough:

if [[ $files == 0 ]] ; then

This probably doesn't do what you expect:

tar xvp -C "$arch_dir" -f "$arch_dir"/archive.tar --strip-components="$sc" 1>&2>>$outfile
printf "\n\n" >>$outfile

# if no error: rm "$arch_dir"/archive.tar
if [ "$?" = "0" ]; then

$? is the exit code of the last command. In this case the printf, and I'm guessing that you're in fact really interested in the tar instead. If you want to act on the exit code of the tar, then you can either use the || operator right after it, or save the exit code, for example:

tar xvp -C "$arch_dir" -f "$arch_dir"/archive.tar --strip-components="$sc" 1>&2>>$outfile
tar_exit=$?
printf "\n\n" >>$outfile

# if no error: rm "$arch_dir"/archive.tar
if [ $tar_exit = 0 ]; then

And no need to quote $? as it's always a number, never blank, never contains spaces.


Copy-paste your code to http://www.shellcheck.net/# and it will point out a couple of more things that you should fix.

\$\endgroup\$
  • \$\begingroup\$ thanks for that link...! Ok, so... a lot of things said don't use echo because it is inconsistent (which I was surprised by). Also, when I write things, my brain can't handle the fact a naked command (like without backticks or - don't kill me - the jquery looking notation) is syntactically correct and will be executed. \$\endgroup\$ – gloomy.penguin Oct 26 '14 at 13:34
  • \$\begingroup\$ i knew $? was something for the last command... but for some unknown reason I didn't consider printf a real command... in my head. I would have never caught that. \$\endgroup\$ – gloomy.penguin Oct 26 '14 at 13:49
  • \$\begingroup\$ About echo versus printf, as I explained above, if all you need is printf 'something\n' then use the simpler echo 'something', if you need anything more sophisticated then use printf \$\endgroup\$ – janos Oct 26 '14 at 14:13
  • 1
    \$\begingroup\$ yeah... I've worked with php before. Seems about the same. I came across several things like this: unix.stackexchange.com/q/65803/82330 while searching commands and stuff. It doesn't really matter; I would have to assume none of the issues listed at that link would affect the (simple) things I am writing right now. All the servers I use have the same rehl version anyway. \$\endgroup\$ – gloomy.penguin Oct 26 '14 at 14:53

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.