3

The following script is supposed to check $1 and see if it is a specific value. It only works when one Bash [[... =~ ...]] regex check is passed in an if statement.

The error is: When more than one check is being performed in an if statement, there is no value deemed okay.

I think it makes sense that doing '! [[...]] || ! [[...]]' might throw an error, but when I go the bloated route and separate the two values to be looked for into two elif lines, it produces the same error.

I would really appreciate some input on this annoyingly simple problem! Thanks :)

This is the script:

c_RESET='\033[0m' c_RED='\033[0;31m' interfaceType="$1" if [ -z "$interfaceType" ]; then echo -e "[${c_RED}ERROR${c_RESET}] the interface type must be specified. (cli/gui)" && exit 1 elif ! [[ "$interfaceType" =~ ^[Cc][Ll][Ii]$ ]] || ! [[ "$interfaceType" =~ ^[Gg][Uu][Ii]$ ]]; then echo -e "[${c_RED}ERROR${c_RESET}] the interface type must be 'cli' or 'gui'" && exit 2 fi echo "You chose the $interfaceType interface type!"
Improve this question
2
  • 5
    Take e.g. ! [[ $x == foo ]] || ! [[ $x == bar ]], and consider what values the different parts of that expression have for various values of $x? Writing a table might help. What does that tell you? What would $x need to be for the whole expression to be false? Commented Nov 17 at 6:45
  • 2
    Canonical Stack Overflow dupe: stackoverflow.com/questions/26337003/… Commented Nov 17 at 15:18

3 Answers 3

Reset to default
10

The main problem with your script is a boolean logic error. In your elif clause you are testing for ("not cli OR not gui") when you should be testing for ("not cli AND not gui").

With OR, the test succeeds if the variable doesn't match either "cli" or "gui", so half of the results will be incorrect. With AND, it succeeds only when it doesn't match both.

This is shown in the truth table below. "NOT cli & "NOT gui" are the conditions being tested, while the AND and OR columns show the results of AND-ing and OR-ing the first two columns.

NOT cli NOT gui AND OR
false false false false
false true false true
true false false true
true true true true

For example:

if [ -z "$interfaceType" ]; then echo -e "[${c_RED}ERROR${c_RESET}] the interface type must be specified. (cli/gui)" && exit 1 elif [[ ! $interfaceType =~ ^[Cc][Ll][Ii]$ && ! $interfaceType =~ ^[Gg][Uu][Ii]$ ]]; then echo -e "[${c_RED}ERROR${c_RESET}] the interface type must be 'cli' or 'gui'" && exit 2 fi

However, that can be improved because regular expressions support alternations with the | pipe character, so you only need to make one regex test. e.g.

if [ -z "$interfaceType" ]; then echo -e "[${c_RED}ERROR${c_RESET}] the interface type must be specified. (cli/gui)" && exit 1 elif [[ ! $interfaceType =~ ^([Cc][Ll][Ii]|[Gg][Uu][Ii])$ ]]; then echo -e "[${c_RED}ERROR${c_RESET}] the interface type must be 'cli' or 'gui'" && exit 2 fi

And can be further improved by setting nocasematch which enables case-insensitive matching for ==, != and =~ operators, as well as in case statements, pattern substitution (e.g. ${parameter/pattern/string}), and programmable completion.

From man bash:

nocasematch If set, bash matches patterns in a case-insensitive fashion when performing matching while executing case or [[ conditional commands, when performing pattern substitution word expansions, or when filtering possible completions as part of programmable completion.

Note that setting nocasematch persists until the end of the current script (or shell if you enter it on the command line). You may want to unset it after the regex test if you want case-sensitive matches for other tests.

shopt -s nocasematch if [ -z "$interfaceType" ]; then echo -e "[${c_RED}ERROR${c_RESET}] the interface type must be specified. (cli/gui)" && exit 1 elif [[ ! $interfaceType =~ ^(cli|gui)$ ]]; then echo -e "[${c_RED}ERROR${c_RESET}] the interface type must be 'cli' or 'gui'" && exit 2 fi

Finally, if you're going to check whether the variable matches neither cli or gui, then you don't also need to check if it's empty - because the empty string doesn't match either of those anyway. So, just the following would do:

shopt -s nocasematch if [[ ! $interfaceType =~ ^(cli|gui)$ ]]; then echo -e "[${c_RED}ERROR${c_RESET}] the interface type must be 'cli' or 'gui'" && exit 2 fi

One last thing, you're using essentially the same echo -e statement twice. If that's something you're likely to use a lot in your script, you should turn it into a function. This will make your script more consistent, and easier to read. Also easier to make changes, e.g. if you want to change the colour for all error messages, or add a timestamp, or whatever.

For example (using tput rather than hard-coding ANSI/vt-100 colour codes, and using printf rather than echo -e, see Why is printf better than echo? - in short, echo -e is non-portable and unreliable):

#!/bin/bash shopt -s nocasematch c_RESET=$(tput sgr0) # turn off all attributes c_RED=$(tput bold setaf 1) # bold red error_exit () { # first arg is the exit code ec=$1 shift # Remaining args ($*) are printed with a red ERROR message. printf "[${c_RED}ERROR${c_RESET}] %s\n" "$*" exit $ec } interfaceType="$1" if [ -z "$interfaceType" ]; then error_exit 1 "The interface type must be specified. (cli/gui)" elif [[ ! $interfaceType =~ ^(cli|gui)$ ]]; then error_exit 2 "The interface type must be 'cli' or 'gui'" fi echo $interfaceType
Improve this answer
5
  • You're described the truth table for a logic function of two variables, but this is a logic function of only one variable.... (The first row in your table should not exist) Commented Nov 17 at 19:19
  • 1
    Context is important. There are two values being tested by the OP, either of which may be true or false, and then combined with OR when it should be AND. For the purpose of that particular truth table and that particular if [[ ... ]] test, it doesn't matter in the slightest that they are potential values for just one variable. However, that is why i recommended simplifying it to just a single regex test with alternation, no need for AND or OR. Commented Nov 18 at 0:41
  • For that particular if test, it absolutely matters that both branches test the same variable. It makes "Not cli" false, "Not Gui" false impossible. Commented Nov 18 at 0:47
  • 3
    As far as that particular table is concerned, it doesn't matter at all that that situation is impossible. The OP's if statement was testing for both conditions, and using OR instead of AND was distorting the result they wanted - that was the point of my answer, and the table only exists to illustrate WHY using OR in that particular test could not produce an accurate or useful result. Commented Nov 18 at 1:06
  • At any rate, learning boolean logic tables is basic stuff for developers, and should always be encouraged. A complete answer is always preferred. Commented 7 hours ago
8

I suspect you can accomplish what you want in bash and in a way that is much easier for others to read and understand.

$ cat t.bash #!/usr/bin/bash croak() { # dying words readonly RED=4 # readability trumps efficiency readonly NORMAL=9 # encapsulate terminal cruftiness tput setb $RED echo -n "ERROR:" tput setb $NORMAL echo " $2" exit $1 } echo -n "What to do (CLI or GUI?): " read interfaceType case ${interfaceType,,} in # converted to lowercase gui) echo "Doing GUI stuff";; cli | cmd) echo "Doing CLI stuff";; cua) croak 3 "IBM Common User Access discontinued";; "") croak 1 "you must enter a choice";; *) croak 2 "You must choose between GUI or CLI";; esac

Some reasons why I think this preferable

  • Using tput ensures the script is portable to different terminal types
  • Putting the error message construction into a function isolates the IO funkiness. (n.b. "separation of concerns")
  • Using a case statement makes the multiple choices much clearer, makes the code simpler to read and more compact
  • You can easily have two or more names for a choice without complex, hard to follow, if statements
  • Doing a conversion to lowercase means you can write the clearer gui rather than [gG][uU][iI] etc
  • You can still have a variety of different validation tests (null choice, unknown choice, etc).
  • Don't worry about efficiency until the code is to be executed a million times a second.
$ bash --version GNU bash, version 5.2.21(1)-release (x86_64-pc-linux-gnu)

results of running script with wrong, missing or correct choice

Improve this answer
5
  • 1
    Personally I think some more detailed explanation of why this is preferred would enhance this answer, but yeah, this is the approach I use in real scripts. Commented Nov 17 at 18:46
  • Thanks for the answer kind user! I will use your method in the future for other projects, but wanted to implement verbose error handling which I forgot to mention in the original post. Thanks for your time :) Commented Nov 18 at 4:15
  • @DavidZ: Answer updated, Not sure exactly what you mean but I gave it a shot. Commented Nov 18 at 15:31
  • @EmberNeurosis: I amended the answer to address what I think you seek. Commented Nov 18 at 15:32
  • I suspect that croak would be improved by exec >&2 early on, given the args seem intended for standard error stream. Commented Nov 18 at 16:50
7

No need to use the Korn-style [[ ... ]] let alone Bash's =~ in there, you could use the standard case construct which would make your code more legible.

RED=$'\e[31m' RESET=$'\e[m' die() { while [ "$#" -gt 0 ] printf>&2 '%s\n' "${RED}ERROR${RESET} $1" shift done exit 1 } if [ "$#" -gt 0 ]; then interfaceType=$1 case $interfaceType in ( [cC][lL][iI] | [gG][uU][iI] ) printf '%s\n' "You chose the $interfaceType interface type!";; ( * ) die 'interface type must be "cli" or "gui"';; esac else die "No interface type specified" fi

That's standard shell syntax. You can shorten it by switching to a shell with builtin case conversion or case insensitive matching, and that has builtin colour support. Zsh would be a better choice than Bash.

autoload colors; colors die() { print -u2 -rC1 -- "$fg[red]ERROR$reset_color $^@" exit 1 } if (( $# )); then interfaceType=$1:l case $interfaceType in ( cli | gui ) print -r "You chose the $interfaceType interface type!";; ( * ) die 'interface type must be "cli" or "gui"';; esac else die "No interface type specified" fi

Beware though that depending on the locale, lower case I may be either ı or i. The above would complain when $1 is CLI in a Turkish locale as clı is neither cli nor gui.

For colouring, here, we're using the colors autoloadable function which defines a few variables including the $fg associative array. You could also use prompt expansion with print -P and the %F{colour} directive:

for psvar {print -ru2 -P '%F{red}ERROR%f %v'}

That would have two advantages:

  • That supports more colours (as many as your terminal supports) as it uses the terminfo interface to do so, and can also find the closest match via the zsh/nearcolor module on terminals that support fewer.
  • When using that %v (short for %1v for the first element of the $psvar array), it gives a visible representation of the string being displayed (for instance, would represent a backspace character as ^H instead of sending it raw to the terminal where it would cause the cursor to move leftward).

$1:l to convert to lower case using the modifier syntax from csh from the late 70s. ${(L)1} using a parameter expansion flag may be more canonical in zsh. bash's equivalent would be ${1,,}. In Korn-like shells (including zsh and bash), you'd use typeset -l interfaceType="$1".

For case insensitive matching, you can use a (#i)(cli|gui) pattern in zsh after the extendedglob option has been enabled (with set -o extendedglob), a ~(i:cli|gui) pattern in ksh93, or enable the nocasematch option in bash (enabled with shopt -s nocasematch).

Note that [ -z "$1" ] tests whether the expansion of $1 is empty. That could be because no argument was passed ($# is 0), or because an empty first argument has been passed (so was specified, only not with one of the right values). To test whether an argument is specified, you should check the value of $# or use things like [ -z "${1+set}" ] or interfaceType=${1?Error message}, not check the length of that argument.

Improve this answer

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.