script- loop through files and directories

Question

I have written the script, which unzips a zip file (passed as argument to the bash), loops through directory and files, then perform different actions based on directory or file. It is working fine.

However I would like to know some things:

1. Is it possible to do this without the unzip command? I somehow feel like that step can be avoided and we can directly use the zip file to perform the task.
2. I have used three temporary files/directories(tmp.txt - to store the directory and file lines, out.xml - for the curl command, DIREC - for finding the files and directories). I have removed those three files at the end of the script. Is there a way to optimize this? may be using variables instead of files.

Here is the bash script, with comments:

#!/bin/bash
#The below line unzips the zip file as passed as argument
unzip 1ドル
length=${#1}
const=4
count=$((length-const))
DIREC=$(echo 1ドル | cut -b 1-$count)
OUTPUTFIL=out.xml
#The below line will find the directory, which was created by the unzip command. Creation of tmp.txt here.
find $DIREC -exec echo {} \; >tmp.txt
exec<tmp.txt
 while read line
 do
if [ -d "${line}" ] ; then
 echo "$line is a directory";
else
 if [ -f "${line}" ]; then
BASE=$(base64 $line);
 echo "${line} is a file";
{
 echo "<inpu>${BASE}</inpu>"
} > ${OUTPUTFIL}
curl -u admin:secret --data-binary @out.xml http://dp1-l2.com; echo
 else
 echo "${line} is not valid";
 exit 1
 fi
fi
 done
rm -f tmp.txt
rm -rf $DIREC
rm -f out.xml

Answer 1

1. There's no documentation. Even a comment at the top of the script summarizing what it does would be better than nothing.

2. There's no error handling. What if the user forgets the argument?

3. This script will break if the argument 1ドル contains a space. You need to write "1ドル" in several places.

4. You don't need to put ; at the end of lines like this:

   echo "$line is a directory";
   

5. This code is really unclear:

   length=${#1}
   const=4
   count=$((length-const))
   DIREC=$(echo 1ドル | cut -b 1-$count)
   

   First, the variable names are poorly chosen: length of what? why const? count of what? Second, this assumes that 1ドル ends with .zip and will go wrong if the file has any other name. Third, Bash has a feature ${parameter%pattern} for removing a suffix from a string, so you can remove the extension from 1ドル like this:

   DIREC=${1%.*}
   

   But fourth and most importantly, how do you know that the ZIP file will unpack into a directory with the same name as the ZIP file? In the example below I create a ZIP file called b.zip that unzips into the directory a:

   $ mkdir a
   $ touch a/a
   $ zip b a/a
    adding: a/a (stored 0%)
   $ rm -r a
   $ unzip b
   Archive: b.zip
    extracting: a/a 
   

   What you should do instead (that is, if you must unzip 1ドル) is to use the -d option to unzip to extract it to a known place.

   $ unzip -d b b
   Archive: b.zip
    extracting: b/a/a 
   

6. Why is OUTPUTFIL misspelled? Why not spell it OUTPUT_FILE?

7. It's a bad idea to put temporary files like $OUTPUTFIL in the current directory. There might already be a file called out.xml in the current directory and then you would end up overwriting it. You should use the mktemp program to create a unique temporary file name or directory, for example like this:

   OUTPUT_FILE=$(mktemp -t unzip-XXXXXX.xml)
   

8. These lines have several problems:

   find $DIREC -exec echo {} \; >tmp.txt
   exec<tmp.txt
   while read line
   

   First, this will go wrong if $DIREC contains a space: you need to use quotes. Second, this will go wrong if $DIREC starts with a - (it will be interpreted as an option): you need to use the -f option to find. Third, there's no need to run a separate echo command for each file: find has a -print option. Fourth, you don't need to write the output to a temporary file in order to read it using while read line; you can pipe the output of find into the while loop. Like this:

   find -f "$DIREC" -- -print | while read line; do
   

9. Inside your while loop you use [ -f ... ] to ensure that you only process plain files (not directories). But you could use the -type f argument to find to ensure that it only outputs plain files:

   find -f "$DIREC" -- -type f -print | while read line; do
   

10. This line will go wrong if $line contains a space or starts with a -:

    BASE=$(base64 $line);
    

    You should write:

    BASE=$(base64 -i "$line")
    

11. It's not a good idea to read the output of the base64 program into a variable like this: the output might be very large and cause Bash to use large amounts of memory. It is better to let base64 write its output directly to your output file, so that it does not have to be stored in a variable in Bash. Instead of:

    BASE=$(base64 $line);
    {
     echo "<inpu>${BASE}</inpu>"
    } > ${OUTPUTFIL}
    

    you should write:

    {
     echo "<inpu>"
     base64 -i "$line"
     echo "</inpu>"
    } > "${OUTPUT_FILE}"
    

12. However, there is no need for an OUTPUT_FILE at all here, because curl accepts the filename - meaning "standard input", so you can write:

    {
     echo "<inpu>"
     base64 -i "$line"
     echo "</inpu>"
    } | curl -u admin:secret --data-binary @- http://dp1-l2.com
    

13. <inpu> looks like a typo to me. Are you sure you don't mean <input>? If <inpu> is correct, you should probably add a comment to explain why.

14. It would be better to put configuration settings like admin:secret and http://dp1-l2.com in named variables at the top of the file. (It might be even better to accept them as command-line arguments.)

15. The way you have written this code means that each individual file in the archive gets uploaded as a separate XML file to the server. Is this really what you want? It seems more likely to me that you would want to upload the whole archive as one XML file. Otherwise, what's the point of using XML?

16. As you suspected, there is in fact no need to unzip the file, because you can use zipinfo -1 to get a listing of the contents, and then use the -p option to unzip to extract a file to standard output.

Putting all this together, I would write the script line this:

#!/bin/bash
# upload-zip.sh -- encode contents of ZIP archive as base64
# strings in an XML document, and upload it.
# Destination and credentials for upload.
UPLOAD_URL=http://dp1-l2.com
USER=admin
PASSWORD=secret
if [ "$#" != 1 ]; then
 echo "Usage: 0ドル zipfile"
 exit 1
fi
ZIPFILE=1ドル
zipinfo -1 -- "$ZIPFILE" | while read FILE; do
 echo "<input>"
 unzip -p -- "$ZIPFILE" "$FILE" | base64
 echo "</input>"
done | curl --user "$USER:$PASSWORD" --data-binary @- --url "$UPLOAD_URL"