7

I'm having some issues with a bash script, but I don't know why. The script is meant to convert the input (in minutes) to seconds and then start counting down until it reaches zero, at which point the script should stop.

When I add a sleep 1 in between, it doesn't count down anymore; why is this happening?

Code:

if [ -z $1 ];
then
     read NUMBER
else NUMBER=$1
fi
SECONDS=$(( NUMBER * 60 ))

while [ $SECONDS -gt 0 ] do sleep 1 SECONDS=$(( SECONDS - 1)) echo $SECONDS remaining done

echo end

Volker Siegel
  • 17,283
NeG
  • 71
  • 2
    Just a word of warning: sleep doesn't guarantee an exact time of sleeping and can be quite inaccurate. – Jack Aidley Jun 25 '23 at 11:20
  • Please modify the tile of this question to include the key fact that the loop (ab)uses a system variable. – Monty Harder Jun 25 '23 at 17:36
  • 7
    @MontyHarder If they'd known that, they wouldn't have had the problem in the first place. The answer shouldn't be in the question/title. – Barmar Jun 25 '23 at 23:14
  • @JackAidley And even if the sleep time were totally accurate, the loop would still get out of time because it doesn't account for the shell processing time etc. (Amusingly, using the shell built-in $SECONDS as intended would work around that!) – gidds Jun 26 '23 at 10:02
  • 1
    This is a great reason not to capitalize variables in scripts. ALL_CAPS should be reserved for environment and special variables. – Josh Jun 26 '23 at 13:35

2 Answers2

20

SECONDS is a variable that's used internally by Bash, and the way this variable works is interfering with your script's decrement action. If you use a different variable name (like SECS or seconds) then it works fine. E.g.:

#!/usr/bin/env bash

if [ -z $1 ]; then read NUMBER else NUMBER=$1 fi

SECS=$(( NUMBER * 60 ))

while [ $SECS -gt 0 ] do sleep 1 SECS=$(( SECS - 1 )) echo $SECS remaining done

echo end

The Bash man page describes the behavior you're seeing with the SECONDS variable:

SECONDS
       Each time this parameter is referenced, it expands to the number of sec-
       onds since shell invocation.  If a value is  assigned  to  SECONDS,  the
       value returned upon subsequent references is the number of seconds since
       the assignment plus the value assigned.  The number of seconds at  shell
       invocation  and  the  current time are always determined by querying the
       system clock.  If SECONDS is unset, it  loses  its  special  properties,
       even if it is subsequently reset.

Since each reference of SECONDS was one second after the previous assignment, the value returned was that second plus the first value assigned (before the while loop). So, in effect, each time through the loop the decrement was being undone.

(also, I added a space between 1 and )) to be consistent with your previous usage)

Sotto Voce
  • 4,131
10

As @SottoVoce said, $SECONDS is a special variable in bash and other Korn-like shells.

In bash and ksh (not zsh), unsetting the variable as with unset -v SECONDS removes its special meaning, but in general, you don't want to use all-uppercase variable names in shell scripts other than for environment ones.

There are a few other issues in your script:

#!/usr/bin/env bash

if [ -z $1 ];

That doesn't make sense. You forgot the quotes around $1, so that's subject to split+glob and empty-removal, so if $1 is empty or unset, that becomes the same as [ -z ] which is the same as [ -n -z ] to check whether -z is a non-empty string, so it will return true then. And if $1 is foo bar or *, that will cause some error. To check whether some arguments are passed or not, you compare the number of arguments ($#) with 0:

if [ "$#" -eq 0 ] # standard
if (( $# == 0 )) # Korn-like shells
then
     read NUMBER

Beware read does some stripping of $IFS characters and some backslash processing, which is probably just fine for a number, but in general to read a line of input, it's:

IFS= read -r minutes
else NUMBER=$1
fi

SECONDS=$(( NUMBER * 60 ))

Using unsanitised data in arithmetic expressions is a command injection vulnerability in bash and other Korn-like shells.

You'd want something like:

case $minutes in
  ("" | *[!0123456789]*) echo>&2 invalid number of minutes; exit 1;;
esac
(( seconds = minutes * 60 ))

To make sure you get a sequence of digits.

while [ $SECONDS -gt 0 ]

bash treats numbers with leading 0s as octal in arithmetic expressions, but not in ['s numeric comparison expressions where they're always considered as decimal, so don't mix and match.

while (( seconds > 0 ))
do
      sleep 1
      SECONDS=$(( SECONDS - 1))
      echo $SECONDS remaining
done

You could also do:

for (( seconds = minutes * 60; seconds > 0; seconds-- )); do
  echo "$seconds seconds remaining"
  sleep 1
done

But note that running commands also takes some time, running sleep 1 1000 times is likely to take at least 1001 seconds as forking a process and running sleep in it would take at least one microsecond, and if you run more commands in your loop, that will drift even more.

To fix that, you could actually use the $SECONDS special variable, but not in bash where it is currently broken and where you can't make it floating point. In zsh instead:

#! /bin/zsh -
(( $# )) || vared -cp 'Enter the number of minutes: ' 1
[[ $1 = <-> ]] || {
  print -u2 Invalid number.
  exit 1
}

(( seconds = 60 * $1 )) typeset -F SECONDS=0 for (( tick = 1; tick <= seconds; tick++ )); do printf '%g seconds remaining\n' seconds-SECONDS (( (delay = tick - SECONDS) <= 0 )) || sleep $delay done