pasadoorian / displaygoat

A crappy shell script that is used to display images on the screen from a specified folder. I will warn, its crappy.
GNU General Public License v3.0
1 stars 0 forks source link

exit value lost due to echo #3

Open krayon opened 7 years ago

krayon commented 7 years ago

The echo command below clobbers "$?", meaning the exit value will instead be the result of the echo command (which doesn't look like your intent):

if [ "$?" -ne 0 ]; then echo "feh command failed, this is a big deal" | tee $OUTPUT exit $? else echo "Success: displaying the images for $SHOW" | tee $OUTPUT fi

One way to do this is to store "$?" ASAP, ie:

if [ "$?" -ne 0 ]; then ret=$? echo "feh command failed, this is a big deal" | tee $OUTPUT exit $ret else echo "Success: displaying the images for $SHOW" | tee $OUTPUT fi

correabuscar commented 6 months ago

The echo command below clobbers "$?", meaning the exit value will instead be the result of the echo command (which doesn't look like your intent):

if [ "$?" -ne 0 ]; then echo "feh command failed, this is a big deal" | tee $OUTPUT exit $? else echo "Success: displaying the images for $SHOW" | tee $OUTPUT fi

One way to do this is to store "$?" ASAP, ie:

if [ "$?" -ne 0 ]; then ret=$? echo "feh command failed, this is a big deal" | tee $OUTPUT exit $ret else echo "Success: displaying the images for $SHOW" | tee $OUTPUT fi

The ret=$? should be before the if otherwise, the if itself overwrites it, so it's like ret=0 because the if...then line itself makes $? be 0 .

To see that you can use set -x:

#!/usr/bin/env bash

OUTPUT="/tmp/some${RANDOM}.log"
SHOW="dummy"

set -xv
false #this gives exit code 1
if [ "$?" -ne 0 ]; then
  ret=$?
  echo "feh command failed, this is a big deal" | tee $OUTPUT
  exit $ret
else
  echo "Success: displaying the images for $SHOW" | tee $OUTPUT
fi

output:

$ ./a.bash
./a.bash:12+ false
./a.bash:13+ '[' 1 -ne 0 ']'
./a.bash:14+ ret=0
./a.bash:15+ echo 'feh command failed, this is a big deal'
./a.bash:15+ tee /tmp/some20419.log
feh command failed, this is a big deal
./a.bash:16+ exit 0

But if you put it before the if:

$ ./b.bash
./b.bash:11+ false
./b.bash:12+ ret=1
./b.bash:13+ '[' 1 -ne 0 ']'
./b.bash:14+ echo 'feh command failed, this is a big deal'
./b.bash:14+ tee /tmp/some20625.log
feh command failed, this is a big deal
./b.bash:15+ exit 1

$ colordiff -up a.bash b.bash

--- a.bash  2024-03-11 15:08:37.693537339 +0100
+++ b.bash  2024-03-11 15:10:11.853538176 +0100
@@ -10,8 +10,8 @@ SHOW="dummy"

 set -x
 false
-if [ "$?" -ne 0 ]; then
-  ret=$?
+ret=$?
+if [ "$ret" -ne 0 ]; then
   echo "feh command failed, this is a big deal" | tee $OUTPUT
   exit $ret
 else

That being said, his script has set -euf -o pipefail and, to offset that set -e, also has something equivalent to false || : before that if test, which means the $? will be 0 due to that : (ie. same as command(s): false || true) (note that PIPESTATUS bash array isn't affected by double || only by single | pipe, so you couldn't use it to see that false's exit code)

So there's two ways I see out of this:

  1. temporarily set +e and not use || :, set -e back after
  2. run that command within the if, but this means you lose it's exact exit code, so you can't use it for an exit $ret (which he doesn't use anyway, so seems preferrable this way) (EDIT:actually you can if you use PIPESTATUS, see below, that d.bash)

So, first case: c.bash

#!/usr/bin/env bash

set -euf -o pipefail #like the original: https://github.com/pasadoorian/displaygoat/blob/1883636decae12e8a90da5344798a096c673427c/displaygoat.sh#L19
OUTPUT="/tmp/some${RANDOM}.log"
SHOW="dummy"

#
#
set -x
#false || : #use 'false' to assume something returned bad exit code (aka 1) // "# Its okay if this fails, so we append the "always true" e.g. !! :

set +e
false
ret=$?
if [ "$ret" -ne 0 ]; then
  echo "feh command failed, this is a big deal" | tee $OUTPUT
  exit $ret  #note though, that in the original he doesn't want to exit because "it's no big deal" ?) we exit anyway I guess.
else
  echo "Success: displaying the images for $SHOW" | tee $OUTPUT
fi
set -e

output:

$ ./c.bash
./c.bash:12+ set +e
./c.bash:13+ false
./c.bash:14+ ret=1
./c.bash:15+ '[' 1 -ne 0 ']'
./c.bash:16+ echo 'feh command failed, this is a big deal'
./c.bash:16+ tee /tmp/some21230.log
feh command failed, this is a big deal
./c.bash:17+ exit 1

Second case: d.bash

#!/usr/bin/env bash

set -euf -o pipefail
OUTPUT="/tmp/some${RANDOM}.log"
SHOW="dummy"

#
#
set -x
#false || : #use 'false' to assume something returned bad exit code (aka 1) // "# Its okay if this fails, so we append the "always true" e.g. !! :

if ! /bin/false; then
  ret="$?" #can't get that exit code here, so this will be '0' due to the 'if .. then' itself!
  echo "feh command failed, this is a big deal" | tee $OUTPUT
  exit $ret  #note though, that in the original he doesn't want to exit because "it's no big deal" ?) we exit anyway I guess.
else
  echo "Success: displaying the images for $SHOW" | tee $OUTPUT
fi

output:

$ ./d.bash
./d.bash:12+ /bin/false
./d.bash:13+ ret=0
./d.bash:14+ echo 'feh command failed, this is a big deal'
./d.bash:14+ tee /tmp/some4293.log
feh command failed, this is a big deal
./d.bash:15+ exit 0

I kept ret=0 above so it's obvious you can't get it via $? however(and this is what I got out of this whole me-making-this-post thing) in this particular case with if ! command; then the exit code of the command can be retrieved via ${PIPESTATUS[0]} (or even just $PIPESTATUS since it has only one element), so this is a "bug fixed" way to do second case: d.bash

#!/usr/bin/env bash

set -euf -o pipefail #like the original: https://github.com/pasadoorian/displaygoat/blob/1883636decae12e8a90da5344798a096c673427c/displaygoat.sh#L19
OUTPUT="/tmp/some${RANDOM}.log"
SHOW="dummy"

#
#
set -x
#false || : #use 'false' to assume something returned bad exit code (aka 1) // "# Its okay if this fails, so we append the "always true" e.g. !! :

if ! /bin/false; then
  #ret="$?" #can't get that exit code here, so this will be '0' due to the 'if .. then' itself!
  ret="${PIPESTATUS[@]}" #but can still get it via PIPESTATUS array for this `if ! command; then` case!
  echo "feh command failed, this is a big deal" | tee $OUTPUT
  exit $ret  #note though, that in the original he doesn't want to exit because "it's no big deal" ?) we exit anyway I guess.
else
  echo "Success: displaying the images for $SHOW" | tee $OUTPUT
fi

output:

$ ./e.bash
./e.bash:12+ /bin/false
./e.bash:14+ ret=1
./e.bash:15+ echo 'feh command failed, this is a big deal'
./e.bash:15+ tee /tmp/some26987.log
feh command failed, this is a big deal
./e.bash:16+ exit 1

(note, obviously change /bin/false to /bin/true to test if the output if the "feh" command succeeds)

All the 5 files mentioned above are here too.