Simple media ingest script

Question

Trying to script a simple command line tool for ingesting media to a target location following a structured naming convention.

It's functional in its current state but I'd like to know if there are things to improve.

#!/usr/local/bin/bash
#make target directory for transfer
make_directory(){
 echo -e "\n\nFollow the prompt to create a project directory.\n\n"
 sleep .5
 while [[ -z "$target_directory" ]]
 do
 echo -e "Path of target directory?"
 read target_directory
 done
 while [[ -z "$brand" ]]
 do
 echo -e "\nBrand Prefix?"
 read brand
 done
 while [[ -z "$project" ]]
 do
 echo -e "\nProject Name?"
 read project
 done
 while [[ -z "$media_type" ]]
 do
 echo -e "\nMedia Type?"
 read media_type
 done
 while [[ -z "$location" ]]
 do
 echo -e "\nLocation?"
 read location
 done
 while [[ -z "$employee" ]]
 do
 echo -e "\nEmployee?"
 read employee
 done
 path=${target_directory}/$(date +'%Y%m%d')_${brand}_${project}_${media_type}_${location}_${employee}
 echo -e "\nCreating directory: ${path}\n\n"
 mkdir -p "${path}"
}
# construct rsync command
construct_rsync(){
 echo -e "\n\nFollow the prompt to construct the rsync command.\n\n"
 while [[ -z "$source_path" ]]
 do
 echo -e "Path to source media?"
 read source_path
 done
 if [[ "$option" == "2" ]]; then
 while [[ -z "$target_directory" ]]
 do
 echo -e "Target directory?"
 read target_directory
 done
 path=$target_directory
 fi
 while true;
 do
 read -p "Additional rsync options? [y/n] " rsync_add
 case $rsync_add in
 [Yy]* )
 echo -e "\nEnter additional rsync parameters:"
 read rsync_options
 break;;
 [Nn]* )
 break;;
 *) echo "Please enter y or no!"
 esac
 done
 echo -e "\nConstructing rsync command...\n"
 sleep .5
 echo -e "Running rsync command:\n
 rsync \n
 -r \n
 --info=progress2 \n
 --log-file=${path}/$(date +'%Y%m%d')_transfer_log.txt \n
 ${rsync_options} \n
 ${source_path}/ \n
 ${path} \n"
 rsync -r --info=progress2 --log-file="${path}/$(date +'%Y%m%d')_transfer_log.txt" ${rsync_options} "${source_path}/" "${path}"
}
# log exit code of rsync
log(){
 echo -e "\nCreating error log..."
 echo $? > "${path}/error_log.txt"
 sleep .5
 if [[ "$?" == "0" ]]; then
 echo -e "\nTransfer complete!"
 elif [[ "$?" != "0" ]]; then
 echo -e "\nError in transfer! Please refer to error_log.txt!"
 fi
}
# read user input and run appropriate functions
while true
do
read -p "Enter [1] to start an ingest or [2] to complete a partial ingest. " option
 case $option in
 1 )
 make_directory
 sleep .5
 construct_rsync
 sleep .5
 log
 break;;
 2 )
 construct_rsync
 sleep .5
 log
 break;;
 * )
 echo "Please enter a valid option!";;
 esac
done

• Sᴀᴍ Onᴇᴌᴀ: Welcome to Code Review! I rolled back your last 2 edits. After getting an answer you are not allowed to change your code anymore. This is to ensure that answers do not get invalidated and have to hit a moving target. If you have changed your code you can either post it as an answer (if it would constitute a code review) or ask a new question with your changed code (linking back to this one as reference). Refer to this post for more information
• usulmuaddib: Thanks! I wasn't familiar with proper etiquette.

Answer 1

Your code is pretty clear and readable. Congratulations!

Here are some suggestions:

1. Stop using sleep! Unless this is for a Hollywood hacker movie, it just slows things down. Nobody thinks delays are cool after the first three times you run something.

2. Take command-line arguments instead of prompting for everything. It's a lot easier to just put stuff into a command line than it is to respond to it at the keyboard. Something like:

   ingest -t $HOME/media -b SONY -p "My Project" 
   

   There are plenty of SO answers on how to do this.

3. Add some more functions! Anything you find yourself doing twice should be a function. Also, anything that you have to "break the flow" in order to do should be a function. Here are some examples:

   while [[ -z "$target_directory" ]]
    do
    echo -e "Path of target directory?"
    read target_directory
    done
   while [[ -z "$brand" ]]
    do
    echo -e "\nBrand Prefix?"
    read brand
    done
   

   That's twice! Write a function to do this:

   target_dir=$(prompt_for_variable target_dir 'Path of target directory?')"
   brand="$(prompt_for_variable brand 'Brand Prefix?')"
   

   Now consider this:

   while true;
   do
    read -p "Additional rsync options? [y/n] " rsync_add
    case $rsync_add in
    [Yy]* )
   

   Right in the middle of "construct_rsync" you stop and loop forever prompting the user for a y/n answer. Write a function for that!

   if get_yn 'Additional rsync options?'
   then
    read -p "Enter the additional options: " rsync_options
   fi
   

4. Don't lie to anybody, especially yourself. You have a function called construct_rsync but what does it do? It runs the command. Either change the name, or change the function.

5. Beware of $?. In your log function you refer to it several times. But $? is updated each time a command is run. So if you're going to make decisions based on the value, you should either capture the value into a separate variable (status=$? ... if $status) or make a single decision right up front (if $? ... else ... fi)

• usulmuaddib: Thanks for the detailed feedback! I don't have much experience with bash or coding in general so it's very helpful to see some better approaches. I do not understand how to implement a user prompt function. I believe i'm misunderstanding bash syntax, how arguments are passed, or missing a piece of the problem. If I write a generalized while loop inside a function it does not run properly when using 1ドル and 2ドル to populate. prompt_user(){ while [[ -z $input ]] do echo "2ドル" read "1ドル" }
• aghast: There's potential problems when mixing while loops with user input, because in some circumstances (depending on bash version, also) the body of the while loop will execute as a sub-shell (separate process, with its own file handles and stuff). Be sure and check Stack Overflow if you're having trouble.
• usulmuaddib: I managed to get my original functionality working and it was a combination of misunderstanding the way variables were passed and some syntactical errors causing it to not work on my end. Appreciate the input!

Answer 2

Consider setting -e and -u to make the script abort on some common failures, rather than wildly continuing. The existing script has almost no error checking; as a simple example, closing standard input will lead to an infinite loop repeatedly executing read.

Instead of using non-standard echo -e to prompt, prefer to supply the prompt as argument to read:

 read -p "Path of target directory? " target_directory

Instead of merely checking that the directory path is a non-empty string with [[ -z ]], we should probably be checking that it's a real directory, with [ -d ].

Output lines should end with newline, and should generally not begin with newline. And error messages should go to the standard error stream (>&2) rather than to standard output.

There are some quotes needed when expanding pathname variables - at present, any filenames including whitespace will be seen as two or more arguments.

Testing $? is an antipattern. This block:

sleep .5
if [[ "$?" == "0" ]]; then
 echo -e "\nTransfer complete!"
elif [[ "$?" != "0" ]]; then
 echo -e "\nError in transfer! Please refer to error_log.txt!"
fi

can be written much more simply as

if sleep .5
then
 echo "Transfer complete!"
else
 echo "Error in transfer! Please refer to error_log.txt!"
fi

(though I suspect you actually meant to test the exit code of a different command to the sleep).

Finally, run Shellcheck on the code. I get far too many warnings (some of which I've already identified above):

shellcheck -f gcc 216201.sh
216201.sh:7:14: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:7:16: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:7:66: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:7:68: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:13:17: note: read without -r will mangle backslashes. [SC2162]
216201.sh:18:22: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:19:13: note: read without -r will mangle backslashes. [SC2162]
216201.sh:24:22: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:25:13: note: read without -r will mangle backslashes. [SC2162]
216201.sh:30:22: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:31:13: note: read without -r will mangle backslashes. [SC2162]
216201.sh:36:22: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:37:13: note: read without -r will mangle backslashes. [SC2162]
216201.sh:42:22: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:43:13: note: read without -r will mangle backslashes. [SC2162]
216201.sh:48:14: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:48:43: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:48:45: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:58:14: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:58:16: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:58:67: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:58:69: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:62:9: note: read without -r will mangle backslashes. [SC2162]
216201.sh:69:13: note: read without -r will mangle backslashes. [SC2162]
216201.sh:76:9: note: read without -r will mangle backslashes. [SC2162]
216201.sh:79:26: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:80:17: note: read without -r will mangle backslashes. [SC2162]
216201.sh:88:14: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:88:45: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:90:36: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:91:11: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:92:8: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:93:22: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:94:59: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:95:22: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:96:21: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:97:13: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:99:87: note: Double quote to prevent globbing and word splitting. [SC2086]
216201.sh:106:14: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:111:11: note: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?. [SC2181]
216201.sh:112:18: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:113:13: note: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?. [SC2181]
216201.sh:114:18: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:122:1: note: read without -r will mangle backslashes. [SC2162]

• usulmuaddib: Excellent feedback! I've restructured a bit of my code per the two answers i've gotten. As I understand it, I've set up the mkdir such that it will create the parent directories if they do not exist. As to the others, I need flexibility in the naming to accommodate any string I just can't have empty strings. How do you suggest I check for empty string and re-prompt the user if the input is empty? I tried to implement a while loop in a function and my inexperience with bash is making progress slow.
• Toby Speight: I may have misunderstood from reading the code whether the target directory had to pre-exist; sorry if I got that wrong. The simple loop seems reasonable to me: until [ "${dir-}" ]; do read -p "Enter directory: " dir; done (the ${dir-} expansion allows $dir to be unset without error despite set -u).