50

I have written a function:

check_log(){
    if [ -f "/usr/apps/appcheck.log" ]
    then
         return 1
    else
         return 0
    fi
}

Then I call this function in an "if" condition:

if [ check_log ];
then
    ........statements....
fi

Will this work? I am confused here because bash returns 0 on success and 1 on failure, but my function is returning 1 and the condition is checking for 1/0, it gets 1 and it should give failures, but in my shell script the condition is passing.

Can anyone shed light on this issue?

Benjamin W.
  • 46,058
  • 19
  • 106
  • 116
JumpOffBox
  • 703
  • 3
  • 10
  • 19
  • What about a number if [ any number ] do something and if [ 0 ] do nothing – JumpOffBox Mar 27 '13 at 22:55
  • In bash, you'd use a numeric context for that -- not `[ ]` or `[[ ]]` but `(( ))` -- like so: `if (( variable )); then ...` will evaluate to true only if `variable` contains a number more than 0. – Charles Duffy Mar 28 '13 at 02:12
  • ...by the way, when "using bash", be sure your shebang is `#!/bin/bash` or `#!/usr/bin/env bash`, not `#!/bin/sh`; if it's the latter, not all bash features will be enabled, so not all advice given here will be valid. – Charles Duffy Mar 28 '13 at 02:35

2 Answers2

80
if [ check_log ];

When you use square brackets you're invoking the test command. It's equivalent to if test check_log which is shorthand for if test -n check_log, which in turn means "if "check_log" is not an empty string". It doesn't call your check_log function at all.

Change it to this:

if check_log;

By the way, the function could be more simply written as:

check_log() {
    ! [ -f "/usr/apps/appcheck.log" ]
}

The return value from a function is the exit status of the last command, so no need for explicit return statements.

John Kugelman
  • 349,597
  • 67
  • 533
  • 578
  • What about a number if [ 6 ] do something and if [ 0 ] do nothing – JumpOffBox Mar 27 '13 at 22:54
  • @JumpOffBox If you want to do numeric tests, you have to use one of the test command's numeric operators, like `if [ 6 -ne 0 ]; then ...`. But note that you cannot use a function in place of one of those numbers; you should think of a shell function as a command (that can succeed or fail), not like a mathematical function (whose primary purpose is to return a value). – Gordon Davisson Mar 28 '13 at 01:51
  • @GordonDavisson Please don't suggest `[ ]` for someone explicitly targeting bash rather than /bin/sh; `(( 7 != 0 ))` is far more readable (and also less error-prone; `(( ))`, like `[[ ]]`, suppresses string-splitting and glob expansion, so quotes aren't necessary to ensure that statements are parsed as intended even with unexpected data values). – Charles Duffy Mar 28 '13 at 02:13
  • So in essence `if check_log;` will check if the return value is false (0) which means "success". – Martin Braun Mar 08 '22 at 12:32
  • 1
    I wouldn't call 0 "false". Exit codes aren't booleans. It's better to think of them as *error codes*. There's one way to succeed but many ways to fail so we encode success as 0 and failures as anything else. – John Kugelman Mar 08 '22 at 14:04
4

As noted by @john-kugelman, one solution (and perhaps the most correct one) is to use the following syntax:

if check_log;

But an alternative solution is:

if [[ $(check_log; echo $?) -eq 0 ]];

My personal preference is the latter as it fosters consistency among conditional statements. But the downside is that because it relies on command substitution, it will fork a child process (i.e. a subshell) where as the first method will not.

An interesting read on the subject can be found here:

When does command substitution spawn more subshells than the same commands in isolation?

Updated (2020-12-01): In most cases it is inadvisable to use the above alternative solution for reasons related to inefficiency and the potential to trigger erroneous results especially if the called function writes to stderr or stdout and that output isn't trapped.

Updated (2017-10-20): Strikeout my preference for the second method as these days I'm on a mission to prevent unnecessary forking. The syntax in the first solution is not as intuitive for non-shell programming, but it certainly is more efficient.

markeissler
  • 649
  • 7
  • 9
  • 3
    Why in the world would you prefer the second one? – John Kugelman Oct 20 '17 at 11:07
  • If you no longer advise the practices given in this answer, you might consider deleting it. We've had new questions [asked by people trying to apply this advice](https://stackoverflow.com/questions/64072970/error-conditional-binary-operator-expected-in-shell-script?noredirect=1#64072970), and it would be better if they didn't. – Charles Duffy Sep 26 '20 at 01:23
  • @CharlesDuffy I don't think it's best practice to actually delete answers that others may have bookmarked. The alternative solution is still correct in syntax it just happens to fork a subshell for evaluation which may or may not be desirable. Spawning subshells is a lightweight process and the pattern is prevalent. – markeissler Nov 30 '20 at 21:25
  • 1
    The answer is not just inefficient, it's also buggy. If `check_log` writes to stdout, anything it writes ends up on the left-hand side of your `-eq` and causes the test to fail, no matter what the actual exit status is. There is *never* a case where this is good practice -- even if you want to explicitly test `$?` (say, to be in a specific range), it's still better to do use different syntax -- such as `if check_log; [ "$?" -lt 3 ]; then`. – Charles Duffy Nov 30 '20 at 23:33
  • @CharlesDuffy Interesting. I didn't consider the case that there might be output to stdout (check_log doesn't do this). Redirecting output could address that case `if [[ $(check_log > /dev/null; echo $?) -lt 3 ]]; then` but still inefficient and prone to the problem you suggest. As to _never_ a case I think that's a bit harsh, perhaps _never in your use cases or perhaps many others_ would be more reasonable. Updating the answer to steer folks in another direction. – markeissler Dec 01 '20 at 21:17