code review fuer shell script

[ben.a]

Hallo,

ich habe eben mein erstes Shell-Script geschrieben:
35101
Sinn des Scripts ist es, alle jpg-Bilder des aktuellen Verzeichnisses mit convert auf eine bestimmte Groesse zu bringen. Die ausgegebenen Bilder sollen in ein zur Laufzeit zu spezifizierendes (zuvor nicht existierendes) Unterverzeichnis gespeichert werden.

Jetzt haette ich gern ein Feedback. Koenntet ihr mich auf Fehler, schlechten Stil etc hinweisen?
Insbesondere hab ich es an einer Stelle nicht geschafft, eine zuvor definierte Variable zu verwenden, anstelle des konkreten Wertes (Zeile 32).

Viele Gruesse,
ben

Edit : Formatierung

[gbotti]

Guten morgen.
Ich habe nicht ganz verstanden für was $SUFFIX in der for-Schleife verwendet werden soll (Zeile 32). Bitte erklär mir das.

Zum Rest vom Script.
Gefällt mir Eventuell würd ich noch eine Prüfung einbauen, ob der Befehl

Code: Alles auswählen

${COMMAND} "${file}" ${ARGUMENT} "${filename}"

erfolgreich war und das ganze evtl. sogar in ein Log-File schreiben.

Dann hast Du folgendes gemacht:

Code: Alles auswählen

...
CUR_DIR=${PWD}
...
GOAL_DIR="${PWD}/$NEW_DIR"
...

Ich würde das so bauen:

Code: Alles auswählen

GOAL_DIR="$CUR_DIR/$NEW_DIR"

Dann brauchst du nämlich nur eine Variable verändern, wenn der Ordner mal anders sein soll.

[rendegast]

Code: Alles auswählen

COMMAND=convert
->
COMMAND=$(which convert)

Code: Alles auswählen

for file in $CUR_DIR/*.jpg

würde ich quoten.

Code: Alles auswählen

$ sh
$ pwd
/tmp/bild
$ SUF=jpg; for file in "$(pwd)"/*.$SUF; do echo $file; done
/tmp/bild/1.jpg
/tmp/bild/2.jpg
/tmp/bild/3.jpg
/tmp/bild/4.jpg
/tmp/bild/5.jpg
/tmp/bild/6.jpg
/tmp/bild/7.jpg
$

Dein ${SUFFIX} ist "small", nicht "jpg"?

[gbotti]

Code: Alles auswählen

COMMAND=$(which convert)

Geile Idee. Das hab ich noch nicht so gemacht...
Nur. Wenn "convert" nicht existiert sollte das hier auch noch abgefangen werden oder?

EDIT: @rendegast: OK. Das mit which is ne richtig geile Sache. Hab das grad ausprobiert, es gibt nichts zurück wenn der Befehl nicht existiert...

[rendegast]

es gibt nichts zurück wenn der Befehl nicht existiert...

Code: Alles auswählen

$ which convert
/usr/bin/convert
$ which conver ; echo $?
1

Code: Alles auswählen

[ -x "${COMMAND}" ] || { echo "Error: You must install ${PACKAGE} for this script to work!"; exit 1; }

[ben.a]

Vielen Dank fuer eure Antworten.
Das mit $SUFFIX hab ich verbockt, ich war wohl zu muede. rendegast macht das, was ich meinte.

Betreffs

Code: Alles auswählen

    
COMMAND=$(which convert)   # a
[ -x "${COMMAND}" ] || { echo "Error: You must install ${PACKAGE} for this script to work!"; exit 1; }   # b

Ich habe gelesen, dass die Option set -e zu Beginn eines Scripts guter Stil sei.
Mit dieser Option endet das Script allerdings schon in Zeile a, falls convert nicht gefunden wird. Es kommt in dem Fall nicht die Fehlermeldung von Zeile b.

Um es mit set -e hinzukriegen, wuerde ich gern folgendes machen:

Code: Alles auswählen

PROGRAM=convert
[ COMMAND=$(which "${PROGRAM}") ] || { echo "Failure" ; exit 1 ; }

Allerdings funktioniert das nicht, es fehlt vielleicht ein Schalter in [ ... ]. Ausserdem scheint die Variable COMMAND ausserhalb der Klammern nicht zu existieren.

ben

[rendegast]

Code: Alles auswählen

[ COMMAND=$(which "${PROGRAM}") ] ...

Das ist Unfug,
in der Form wird getestet, ob "COMMAND=/usr/bin/convert" respektive "COMMAND=" ist.
Wäre also immer war.
Die eigentliche Schreibweise dafür ist

Code: Alles auswählen

[ -n irgendein_String ] ...

Ein (vermutlich beabsichtigter) String-vergleich wäre

Code: Alles auswählen

[ COMMAND = $(which "${PROGRAM}") ] ...

also ob "COMMAND" gleich "/usr/bin/convert" ist, was aber immer falsch ist.
Weiterhin wäre möglich, daß der Test so aussieht:

Code: Alles auswählen

$ [ COMMAND =  ] ; echo $?
bash: [: COMMAND: Einstelliger (unärer) Operator erwartet.
2
# Der Test selbst ist fehlerhaft

# Hingegen definiert wäre es mit Quotes:
$ [ COMMAND = "" ] ; echo $?
1

Gemeint ist wohl

Code: Alles auswählen

[ "$COMMAND" = "$(which "${PROGRAM}")" ] ...

und mit dem allgemein üblichen Mittel der formalen Leerstring-Vermeidung:

Code: Alles auswählen

[ "x$COMMAND" = "x$(which "${PROGRAM}")" ] ...

(jedoch hier schlecht, weil eventuell beide Variablen leer sich könnten,
 dann der Test aber ein nichtbeabsichtigtes POSITIV ergibt)

wobei sich mir jedoch der Sinn des Test nicht erschließt?




--------------------------------
Quoten ist Tests ist wirklich wichtig, ein Beispiel: [ -x $Variable ]

Code: Alles auswählen

$ [ -x  ] ; echo $?
0
# Ein "Nichts" wäre ausführbar? (Das '-x' wird hier wohl wieder als nichtleerer String interpretiert)

# So ist es richtig:
$ [ -x "" ] ; echo $?
1

[ben.a]

Sorry, ich hab mich unverstaendlich ausgedrueckt. Mein Ziel ist es, das Skript mit der Option "-e" auszufuehren.
Du hattest vorgeschlagen:

Code: Alles auswählen

COMMAND=$(which convert)
[ -x "${COMMAND}" ] || { echo "Error: You must install ${PACKAGE} for this script to work!"; exit 1; }

Bei Benutzung der Option "-e" (set -e) gibt jedoch schon die Wertzuweisung "1" zurueck, und das Skript endet vorzeitig, ohne die zweite Zeile ueberhaupt auszufuehren. Die Fehlermeldung wird also nie ausgegeben.
Ich wuerde also die erste Zeile gern in einen Fehlerabfang wrappen.
Eine Loesung waere

Code: Alles auswählen

if $(which convert) ; then COMMAND=$(which convert); else Fehlermeldung

aber da hab ich zweimal which, was ich gern vermeiden wuerde.
Mein Vorschlag war Unsinn, weil das "=" als Vergleich und nicht als Zuweisung interpretiert wird.

[rendegast]

Bei Benutzung der Option "-e" (set -e) gibt jedoch schon die Wertzuweisung "1" zurueck, und das Skript endet vorzeitig, ohne die zweite Zeile ueberhaupt auszufuehren. Die Fehlermeldung wird also nie ausgegeben.

Sorry,

Dazu, das 'set -e' muß ja nicht immer vorne stehen, zBsp /etc/init.d/binfmt-support:

Code: Alles auswählen

...
### END INIT INFO

PATH=/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin
NAME=binfmt-support
DESC="additional executable binary formats"

if [ "$(uname)" != Linux ]; then
  exit 0
fi

which update-binfmts >/dev/null 2>&1 || exit 0

. /lib/lsb/init-functions
[ -r /etc/default/rcS ] && . /etc/default/rcS

set -e
CODE=0
...

und dann gibt es noch dieses niedliche, /etc/init.d/dovecot:

Code: Alles auswählen

...
# Do NOT "set -e"
...

[ben.a]

Dazu, das 'set -e' muß ja nicht immer vorne stehen

Manche behaupten sogar, die Benutzung sei generell zu vermeiden:
http://bugs.debian.org/139969

Nochmals Danke fuer eure Geduld und Hilfe.
ben