3
\$\begingroup\$

I just got through writing a bash script so my friend and I can share snippets of code to a central file via rsync and ssh. What it does is this;

  1. Sets up ssh public key auth if not already working.
  2. Makes a local "snippet file" using rsync to copy an exiting file from remote server.
  3. Allows the user to edit the snippet file.
  4. If user chooses, sync snippet file to remote host.

I put the project on github at https://github.com/shakabra/snippetShare

It's a pretty straight-forward script, but I wonder what could be done better.

I am really new at programming and was hoping you guys might spot some obvious mistakes that I just over-looked or don't understand.

#!/bin/bash
SERVER="dakinewebs.com"
UN=""
PORT="22"
TIME=$(date +"%c")
localSnippetFile=$HOME/snippetFile.html
remoteSnippetFile=/home/shakabra/dakinewebs.com/snippetShare/snippetFile.html
checkSsh () { 
 ssh -o BatchMode=yes "$UN"@"$SERVER" 'exit'
 if [[ $? = 255 ]]; then
 echo "Local key doesn't exits"
 echo "creating local key"
 ssh-keygen -t rsa
 ssh-add
 uploadKey
 fi
}
sshConfig () {
 if [[ -z $SERVER ]]; then
 echo -e "SERVER NAME:\n"
 read SERVER
 fi
 if [[ -z $UN ]]; then
 echo -e "USERNAME:\n"
 read UN
 fi
 if [[ -z $PORT ]]; then 
 echo -e "PORT:\n"
 read PORT
 fi 
}
makeLocalSnippet () {
 echo "syncing with remote snippet"
 rsync -avz --progress -e ssh "$UN"@"$SERVER":"$remoteSnippetFile" "$localSnippetFile" 
}
editSnippet () {
 echo -e "What's the title of the snippet?\n"
 read snippetTitle
 echo -e "<time>"$TIME"</time>\n" >> "$localSnippetFile"
 echo -e "<header>"$snippetTitle"</header>" >> "$localSnippetFile"
 echo "<pre></pre>" >> "$localSnippetFile"
 nano "$localSnippetFile"
 syncSnippet
 while [[ $? = 1 ]]; do
 syncSnippet
 done
}
syncSnippet () {
 read -p "Sync snippet file now [y/n]: " syncNow
 case $syncNow in
 y|Y|yes|YES|Yes)
 rsync -av --delete -e ssh "$localSnippetFile" "$UN"@"$SERVER":"$remoteSnippetFile"
 ;;
 n|N|no|No|NO)
 echo "Guess we'll sync next time. Hope it wasn't too important!"
 ;;
 *)
 echo "not an answer"
 return 1
 ;;
 esac
}
uploadKey () 
{
 ssh-copy-id "$UN"@"$SERVER" 
 ssh "$UN"@"$SERVER" 'exit'
 echo "Key uploaded"
} 
sshConfig
checkSsh
makeLocalSnippet
editSnippet
asked May 24, 2012 at 4:18
\$\endgroup\$
3
  • 4
    \$\begingroup\$ You should show your code in the question, rather than giving a link to the code, as per faq. \$\endgroup\$ Commented May 24, 2012 at 10:47
  • \$\begingroup\$ I have not looked at the code, but it just seems to me that you would be better off just using a version control system. \$\endgroup\$ Commented May 24, 2012 at 22:33
  • \$\begingroup\$ If you are satisfied with one of the answer, it is nice to "accept" the answer :). \$\endgroup\$ Commented May 29, 2012 at 8:34

2 Answers 2

4
\$\begingroup\$

Your read calls perform \ expansion: if the line ends with \, the shell continues reading the next line and returns the text without the backslash-newline sequence; any other backslash quotes the next character (i.e. the backslash is ignored, except that \\ yields \). This is probably not desirable; use read -r to avoid this behavior. Also, for future reference, note that whitespace at the beginning and end of the input is stripped (you need to set IFS to the empty string to avoid this, but here stripping whitespace is desirable); see Understanding IFS and the linked posts.

Furthermore, since this script is designed for interactive use, you should use read -e to enable editing of the input line (with readline). It would also be nicer to have the prompt together with the input line: use read -p 'Server name: ' and so on instead of calling echo (and don't SHOUT). For example:

if [[ -z $SERVER ]]; then
 read -rep 'Server name: ' SERVER
fi
rsync -avz --progress -e ssh "$UN"@"$SERVER":"$remoteSnippetFile" "$localSnippetFile"

This looks a little more complicated than it needs to be, you can write "$UN@$SERVER:$remoteSnippetFile".

The following part has a bug and is not as readable as it could be:

echo -e "<time>"$TIME"</time>\n" >> "$localSnippetFile"
echo -e "<header>"$snippetTitle"</header>" >> "$localSnippetFile"
echo "<pre></pre>" >> "$localSnippetFile"

The bug is that $TIME and $snippetTitle undergo word splitting and filename generation (i.e. globbing). For example, if $snippetTitle is The A* algorithm and the current directory contains the files Astar.tex, Astar.pdf and test.c, then you're writing <header>The Astar.pdf Astar.tex algorithm</header> to the file. Always use double quotes around variable and command substitutions, unless you know why you must leave them off and why it's safe to do so. (Exception: you may leave off the double quotes in contexts where they are explicitly not needed, such as inside [[ ... ]].)

The readability improvement is to use a here document.

cat <<EOF >>"$localSnippetFile"
<time>$TIME</time>
<header>$snippetTitle</header>
<pre></pre>
EOF
nano "$localSnippetFile"

glenn jackman has already mentioned this: it's rude to impose your editor on the user. There is a standard for letting the user choose an editor: use the VISUAL environment variable, falling back to EDITOR. Use a sensible fallback if neither variable is set.

if [[ -z ${VISUAL=$EDITOR} ]]; then
 for VISUAL in sensible-editor nano vi; do
 if type "$VISUAL" >/dev/null 2>/dev/null; then break; fi
 done
fi
Drop this ...
 while [[ $? = 1 ]]; do
 syncSnippet
 done

... and instead change syncSnippet to first call read until the user enters an accepted input, and then perform the actions based on this input.

while
 read -p "Sync snippet file now [y/n]: " -rN1 yn
 [[ $yn != [YNyn] ]]
do :; done
if [[ $yn = [Yy] ]]; then
 rsync ...
fi
answered May 24, 2012 at 23:12
\$\endgroup\$
1
  • \$\begingroup\$ Wow! thanks Giles and glen. I have got a lot of homework to do now. In particular thanks for the help with that syncSnippet logic. I appreciate the time you guys put in to school me. \$\endgroup\$ Commented May 25, 2012 at 23:27
3
\$\begingroup\$

Don't hardcode the editor. I happen to prefer vi, so don't force me to use nano.

editor=${VISUAL:-${EDITOR:-nano}}
answered May 24, 2012 at 10:12
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.