8
\$\begingroup\$

I'm writing a bash script to handle the PPA's on Ubuntu & derivatives. I was wondering how to know if I'm handling this the right way.

So, the script works (flawlessly), but I posted it to a blog and got this feedback :

The way you wrote you script is the one we all use when we begin : we don't know the variables, so we use traditionnal commands, and in the end if we remove the "#" arguments from the script, the code is sad (maybe he meant 'poor' ?)

Here is a sample :

#! /bin/bash
SCRIPT="ppa-tool"
VERSION="2.0.1" 
DATE="2014-03-26"
RELEASE="$(lsb_release -si) $(lsb_release -sr)"

[ -n "`echo "$1" | grep ppa`" ] && PPA=$1
[ -n "`echo "$2" | grep ppa`" ] && PPA=$2

helpsection() 
{ 
echo -e "Usage : $SCRIPT [OPTION]... [PPA]... 
-h, --help     shows this help
-c, --check    check if [PPA] is available for your release

Version $VERSION - $DATE"
exit 0
}

error() { echo -e "$SCRIPT - Oops, something went wrong\nTry « $SCRIPT --help » for more information." && exit 0 ; }

ppa_verification()
{ 
wget http://ppa.launchpad.net/$(echo $PPA | sed -e 's/ppa://g')/ubuntu/dists -O /tmp/"$SCRIPT-check.tmp" -q 
if [[ -n "`cat "/tmp/$SCRIPT-check.tmp" | grep $(lsb_release -sc)`" ]] ; then 
    echo -e "$SCRIPT : '$PPA' is available for $RELEASE"
else 
    echo -e "$SCRIPT : '$PPA' is NOT available for $RELEASE"
fi  
rm "/tmp/$SCRIPT-check.tmp"
}

[ "$1" == "--help" ] || [ "$1" == "-h" ] && helpsection

if [ "$1" == "--check" ] || [ "$1" == "-c" ] || [ "$1" == "check" ]  ; then
    [ -n "`echo $2`" ] && ppa_verification && exit 0
    error
fi

error

(complete script - also in French)

Apparently, my syntax is bad, but I don't see what I could do to improve this, and I don't understand the feedback I recieved. Could you help me figure it out ?

EDIT: From Unix&Linux (where I first posted, but was redirected here), someone said it wasn't good to use capital letters for variables. So $SCRIPT should become $script.

\$\endgroup\$
  • \$\begingroup\$ @Jamal : are you sure this is a better title than "Bash script : how to know if it's written correctly?" ? Because if the script itself is for PPA's handling, the main reason I'm here is to find out where I'm wrong with my BASH, not with the stuff the script does at the end \$\endgroup\$ – MrVaykadji Mar 26 '14 at 22:58
  • 3
    \$\begingroup\$ I can assure that this title is better! Here we are doing review so asking for : how to know if it's written correctly is not relevant. And you should not include tag in the title. So yes the title is better. \$\endgroup\$ – Marc-Andre Mar 26 '14 at 23:02
  • \$\begingroup\$ OP edited, now the full script is in English, should help the reviewers. \$\endgroup\$ – MrVaykadji Mar 26 '14 at 23:23
7
\$\begingroup\$

User experience

  • -h or --help should print the usage message to standard output, which you do. However, on any unrecognizable command line, you should print the same (or similar) message to standard error and exit with a non-zero status.
  • Similarly, other error messages should go to standard error and result in a non-zero status.

Style issues

  • Good job, double-quoting almost every variable you used. A lot of shell programmers neglect to do so.
  • Good job, breaking the program up into functions. A lot of shell programmers don't.
  • Indent your code consistently. It's hard to see where your functions begin and end.
  • Make use of case to replace convoluted if statements.
  • Do all of your command-line parsing in one place, and have a coherent parsing strategy. You can either use getopt (which has a bit of a learning curve) or build something based on shift (which is easier for a beginner to understand).
  • You can pass parameters to functions. Passing $PPA to ppa_verification() would be more elegant than using a global.

There isn't 100% consensus on whether variable names should be $ALL_CAPS or $lower_case. One common convention is $ALL_CAPS for global and/or exported variables, $lower_case for local or non-exported variables.

Use of wget

  • Instead of writing the output of wget to a temporary file, you could just pipe it to grep directly: wget -q -O - "$url" | grep -q "$release".
  • But then, to distinguish between a wget failure (e.g. network problem) and the release not being found by grep, you would have to interrogate ${PIPESTATUS[0]} and ${PIPESTATUS[1]}, which is a bit complicated.
  • Why not try to fetch the subdirectory of dists/ that you care about instead? Then you only need to test whether the server returned an HTTP 200 (Success!) or HTTP 404 (Not Found).
#! /bin/bash
SCRIPT="ppa-tool"
VERSION="200_success"
DATE="2014-03-26"
RELEASE="$(lsb_release -si) $(lsb_release -sr)"

helpsection() 
{ 
    echo "Usage : $SCRIPT [OPTION]... [PPA]... 
-h, --help     shows this help
-c, --check    check if [PPA] is available for your release

Version $VERSION - $DATE"
}

ppa_verification()
{ 
    local ppa="${1#ppa:}"

    local codename="$(lsb_release -sc)"
    local url="http://ppa.launchpad.net/$ppa/ubuntu/dists/$codename/"

    wget "$url" -q -O /dev/null
    ######################################################################
    # Exit Status
    #
    # Wget may return one of several error codes if it encounters problems.
    # 0 No problems occurred.
    # 1 Generic error code.
    # 2 Parse error--for instance, when parsing command-line options, the `.wgetrc' or `.netrc'...
    # 3 File I/O error.
    # 4 Network failure.
    # 5 SSL verification failure.
    # 6 Username/password authentication failure.
    # 7 Protocol errors.
    # 8 Server issued an error response.
    ######################################################################
    case $? in
      0) # Success
        echo "$SCRIPT : '$ppa' is available for $RELEASE"
        ;;
      8) # HTTP 404 (Not Found) would result in wget returning 8
        echo "$SCRIPT : '$ppa' is NOT available for $RELEASE"
        return 1
        ;;
      *)
        echo "$SCRIPT : Error fetching $url" >&2
        return 3
    esac
}

PPA=
while [ -n "$*" ] ; do
    case "$1" in
      -h|--help)
        helpsection
        exit 0
        ;;
      --check=*)
        PPA="${1#*=}"
        ;;
      -c|--check|check)
        PPA="$2"
        shift
        ;;
      *)
        helpsection >&2
        exit 2
        ;;
    esac
    shift
done

if [ -z "$PPA" ]; then
    helpsection >&2
    exit 2
fi

ppa_verification "$PPA"
\$\endgroup\$
  • \$\begingroup\$ Thank you !! I don't understand everything but I'll make my homeworks \$\endgroup\$ – MrVaykadji Mar 28 '14 at 8:18
  • \$\begingroup\$ There's a little mistake in your $url (from ppa_verification), it should be local url="http://ppa.launchpad.net/$ppa/ubuntu/dists/$codename/" \$\endgroup\$ – MrVaykadji Mar 28 '14 at 22:01
  • \$\begingroup\$ A mistake in my code or your your code? I thought I was silently fixing a bug. \$\endgroup\$ – 200_success Mar 28 '14 at 22:02
  • \$\begingroup\$ Yours. The suggested method is great, but you made a mistake with the url, it shouldn't have '/ppa' inside it. \$\endgroup\$ – MrVaykadji Mar 28 '14 at 22:13
  • 1
    \$\begingroup\$ it is specified by the user, always. The convention is ppa:user/directory. With apt, for example apt-add-repository ppa:user/directory, and it is the same with this script. \$\endgroup\$ – MrVaykadji Mar 29 '14 at 9:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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