Is this shell script for copying files sensible, readable?

Question

As an exercise in learning bash, I wrote this script designed to automate the process of populating a drive with uncompressed .aiff files copied directly from a CD. It saves me having to do a bunch of separate commands to create subdirectories, check before clobbering, etc.

It stores them in this fashion:

Root directory -> Artist -> Album -> Disc in the series (if necessary)

I wrote it to be run in bash on a Mac. Hence the "open" command.

1. What best practices am I ignoring?

2. Is any of the logic unnecessarily complex? (Aside from the whole endeavor, which I suppose I could just do through the GUI... but where's the fun in that?)

Specifically, I wonder about comparing the contents of an origin directory to a destination directory by outputting the destination's content to a text file and running an inverse grep search on it:

for trackname in $(ls -1 /Volumes/$the_disc | grep -vf ~/.harvest/existing); do

Thanks for any feedback you can offer this newbie.

#!/bin/bash

# > harvest.sh <
# Identifies audio CDs and copies their contents to my archive directory
# Written by parisminton for Concrete Daydreams.
# <[email protected]>

vrs='v 0.7'
lastchange='8/15/11'

echo -e "\n--> Harvest $vrs $lastchange <--"

# ...change IFS to newline...
OLD_IFS=$IFS
IFS=$'\n'

i=0

tryagain="--> I can't find any more external volumes. Are you sure you inserted a CD or external drive?\n"

function y_or_n () {
  until [ "$pick" == "y" -o "$pick" == "n" ]; do
    echo -e "--> (Enter \"y\" or \"n\".)\n"
    read pick
  done
}

function collect () {
  the_disc=$1
  multi=null
  ls /Volumes/$1
  echo -e "\n--> What's the artist's name?\n"
  read artist
  echo -e "\n--> OK. We're storing music from $artist."

  echo -e "\n--> Is \"$1\" the name of the album?"
  pick=null
  y_or_n

  if [ "$pick" == "y" ]; then
    echo -e "\n--> Excellent. $1 is the name of the album by $artist.\n"
    album=$1
  else
    echo -e "\n--> What's the name of the album?\n"
    read album
    echo -e "\n--> Excellent. $album is the name of the album by $artist."
  fi

  echo -e "\n--> Is \"$album\" a multi-disc set?"
  pick=null
  y_or_n

  if [ "$pick" == "y" ]; then
    echo -e "\n--> Which disc is this? (\"1,\" \"Disc 1,\" \"The Love Below,\" \"etc...)\n"
    read multi
    if [[ "$multi" =~ ^[0-9]+$ ]]; then
      multi="Disc $multi"
    fi
  fi
}

function archive () {
  musicpath="/path/to/music/directory/"

  if [ ! -e "$musicpath/$artist" ]; then 
    mkdir "$musicpath/$artist"
    echo -e "\n--> I just created a folder named \"$artist.\""
  else
    echo -e "\n--> $artist already has a folder in the archive."
  fi
  cd "$musicpath/$artist"

  if [ ! -e "$musicpath/$artist/$album" ]; then
    mkdir "$musicpath/$artist/$album"
    echo -e "\n--> I just created a folder named \"$album.\""
  else
    echo -e "\n--> The album \"$album\" already has a folder in the archive."
  fi
  cd "$musicpath/$artist/$album"

  if [ "$multi" != null ]; then
    if [ ! -e "$musicpath/$artist/$album/$multi" ]; then
      mkdir "$musicpath/$artist/$album/$multi"
      echo -e "\n--> I just created a folder named \"$multi.\""
    else
      echo -e "\n--> \"$multi\" already has a folder in the archive."
    fi
    cd "$musicpath/$artist/$album/$multi"
  fi

  mkdir ~/.harvest
  ls -1 . > ~/.harvest/existing
  tracktotal=$(ls -1 /Volumes/$the_disc | grep -cvf ~/.harvest/existing)

  if [ $tracktotal -gt 0 ]; then
    open .
    trackcount=0
    for trackname in $(ls -1 /Volumes/$the_disc | grep -vf ~/.harvest/existing); do
      trackcount=$(($trackcount+1))
      echo -e "\n--> Copying \"$trackname.\" This may take a minute or two..."
      cp "/Volumes/$the_disc/$trackname" .
    done
    echo -e "\n--> Finished copying."
    echo -e "\n--> $tracktotal tracks from the album \"$album\" by $artist have been saved to $(pwd)."
  else
    echo -e "\n--> Nothing copied. All the files on $album have already been archived.\n"
  fi

  rm -PR ~/.harvest
}

# ...compare the /Volumes list to the pre-defined list of internal volumes
# we know aren\'t the audio CD; any inverse matches are likely to be the
# CD we want to archive...
if [ "$(ls -1 /Volumes | grep -vf ~/.int_vols)" ]; then

  for filename in $(ls -1 /Volumes | grep -vf ~/.int_vols); do
    if [ -d "/Volumes/$filename" ]; then
      fn[$i]=$filename
      i=$(($i+1))
    fi
  done

  if [ ${#fn[*]} -gt 1 ]; then
    echo -e "\n--> I see more than one volume that might contain the music you want to archive:\n"

    PS3="--> Which one do you want? (Type the number of your choice.) "

    select disc in ${fn[*]}; do
      echo -e "\n--> Cool. You selected $disc.\n"
      break
    done

    collect $disc
    archive

  else
    echo "\n--> Does the volume named \"${fn[0]}\" contain the music you want to archive?"    
    y_or_n
    if [ "$pick" == "y" ]; then
      collect ${fn[0]}
    else
      echo $tryagain
    fi
  fi

else
  echo $tryagain
fi

# ...restore IFS...
IFS=$OLD_IFS

Answer 1

Some comments:

1. You script does not work if you have spaces in the filenames, but I guess this cannot happen.
2. You do not need to restore IFS as the script will be run in a subprocess, and will not impact parent environment.
3. You could run ls -1 /Volumes | grep -vf ~/.int_vols only once, doing a filenames=$(ls -1 /Volumes | grep -vf ~/.int_vols) before the if.
4. You could save the -d tests in the for loop by using find /Volumes -mindepth 1 -maxdepth 1 -type d | cut -d "/" -f 3 instead of ls -1 /Volumes.
5. You could save the entire for loop doing a find /Volumes -mindepth 1 -maxdepth 1 -type d | cut -d "/" -f 3 | paste -s -d " " | read -a f

The cut -d "/" -f 3 does not work well if you change /Volumes by a directory that is not directly under /. I would use sed -e s|.*/|| instead, but this introduces a new tool that you might not be familiar with.

For the rest of the script, I do not know the structure of the data, so I cannot tell.