Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This is my very first Bash script that does something real. I'm here to learn, so please tell me if and how this can be done better.

#!/bin/sh

# Daniele Brugnara
# October - 2013

if [ -k $1 ]; then
        echo "Error! You must pass a repository name!"
        exit 1
fi

SVN_PATH="/var/lib/svn"
currPath=$(pwd)

cd $SVN_PATH
svnadmin create $1
chown -R www-data:www-data $1
cd $currPath

echo "Done! Remember to set valid user permission in authz file."
share|improve this question
add comment

1 Answer

up vote 3 down vote accepted
  1. If the first argument to your script contains a space, then you get an error message:

    $ ./cr32076.sh "my repository"
    ./cr32076.sh: line 6: [: my: binary operator expected
    

    To avoid this, put quotes around the variable $1, like this:

    if [ -k "$1" ]; then
    
  2. The man page for [ says:

    -k file       True if file exists and its sticky bit is set.
    

    is this really what you mean? It seems more likely that you want the -z option:

    -z string     True if the length of string is zero.
    
  3. You remember the current directory in a variable:

    currPath=$(pwd)
    

    and then later change to it:

    cd $currPath
    

    but this will go wrong if the current directory has a space in its name:

    $ pwd
    /Users/gdr/some directory
    $ currPath=$(pwd)
    $ cd $currPath
    bash: cd: /Users/gdr/some: No such file or directory
    

    To avoid this, put quotes around the variable, like this:

    cd "$currPath"
    
  4. If you want to change directory temporarily and change back again, then instead of writing

    currPath=$(pwd)   # remember old directory
    cd "$SVN_PATH"    # change to new directory
    # do some stuff
    cd "$currPath"    # go back to old directory
    

    you can use a subshell:

    (
        cd "$SVN_PATH"    # change to new directory
        # do some stuff        
    ) # old directory is automatically restored when subshell exits
    

    But in this script, there's no need to change directory back again, because after you go back all you do is echo "Done! ..." and it doesn't matter what directory you do that in.

  5. In fact, there's no need to change directory at all, because instead of

    SVN_PATH="/var/lib/svn"
    currPath=$(pwd)
    
    cd "$SVN_PATH"
    svnadmin create "$1"
    chown -R www-data:www-data "$1"
    cd "$currPath"
    

    you could write

    SVN_PATH="/var/lib/svn/$1"
    svnadmin create "$SVN_PATH"
    chown -R www-data:www-data "$SVN_PATH"
    
share|improve this answer
 
Thanks. This is a very useful answer. –  Daniele Brugnara Oct 2 '13 at 7:17
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.