[PATCH] confusing/obsolete handling of test -t operator (and doc warnings against using -o/-a)

[Stephane Chazelas]

Hello,

test -t X

Always returns false and doesn't report an error about that
invalid number (beside the point here, but in ksh/zsh, that X is treated
as an arithmetic expression and evaluates to 0 if $X is not set).

While:

test -t X -a Y

returns a "too many arguments" error.

test X -a -t -a Y

returns false (without error and regardless of whether any fd is a tty)
while

test X -a Y -a -t

returns true

While for other unary operators that gives:

$ bash -c 'test X -a -x -a Y'
bash: line 1: test: too many arguments

No big deal as in all those the behaviour is unspecfied by
POSIX (non-numeric argument to -t, or more than 4 arguments, -a
deprecated).

It seems to be explained by what looks like a remnant from the
time where [ -t ] was short for [ -t 1 ] in the code of
unary_operator() in test.c

>  /* the only tricky case is `-t', which may or may not take an argument. */

Not anymore.

>  if (op[1] == 't')
>    {
>      advance (0);
>      if (pos < argc)
>       {
>         if (legal_number (argv[pos], &r))
>           {
>             advance (0);
>             return (unary_test (op, argv[pos - 1], 0));
>           }
>         else
>           return (FALSE);

Maybe the intention was to do a isatty(1) here instead of always
returning false, but that and the fact that advance() is not called only
confuses things.

>       }
>      else
>       return (unary_test (op, "1", 0));

That part is never reached AFAICT as unary_operator() is never
called with pos == argc.

I beleive that whole code can go as -t is now always a unary
operator, and it would be more useful to report an error when
the operand is not a number.

I also noticed that the fact that -a/-o were deprecated (by POSIX at
least) and made for unreliable test expressions was not noted in the
manual. So I suggest the patch below:

diff --git a/doc/bashref.texi b/doc/bashref.texi
index 85e729d5..00fbab69 100644
--- a/doc/bashref.texi
+++ b/doc/bashref.texi
@@ -4215,14 +4215,14 @@ Operator precedence is used when there are five or more 
arguments.
 @item ! @var{expr}
 True if @var{expr} is false.
 
-@item ( @var{expr} )
+@item ( @var{expr} ) (DEPRECATED)
 Returns the value of @var{expr}.
 This may be used to override the normal precedence of operators.
 
-@item @var{expr1} -a @var{expr2}
+@item @var{expr1} -a @var{expr2} (DEPRECATED)
 True if both @var{expr1} and @var{expr2} are true.
 
-@item @var{expr1} -o @var{expr2}
+@item @var{expr1} -o @var{expr2} (DEPRECATED)
 True if either @var{expr1} or @var{expr2} is true.
 @end table
 
@@ -4283,11 +4283,26 @@ Otherwise, the expression is parsed and evaluated 
according to
 precedence using the rules listed above.
 @end enumerate
 
-@item 5 or more arguments
+@item 5 or more arguments (DEPRECATED)
 The expression is parsed and evaluated according to precedence
 using the rules listed above.
 @end table
 
+In the 4 or 5 arguments case, the use of @samp{(}, @samp{)}, binary
+@samp{-a}, binary @samp{-o} make for unreliable test expressions. For
+instance @code{test "$x" -a ! "$y"}  becomes a test for whether a
+@samp{!} file exists if @code{$x} is @samp{(} and @code{$y} is
+@samp{)} and @code{[ -f "$file" -a ! -L "$file" ]} fails with a
+syntax error for a file called @samp{==}. Which explains why those
+are deprecated as they have been in the POSIX specification of the
+@code{test} utility since 2008.
+
+Each invocation of @code{[} / @code{test} should perform a single test
+and several invocations may be chained with the @code{&&} or @code{||}
+shell operators to achieve the same result as the @code{-a} and
+@code{-o} operators reliably as in @code{test "$x" && test ! "$y"} or
+@code{[ -f "$file" ] && [ ! -L "$file" ]} in the examples above.
+
 When used with @code{test} or @samp{[}, the @samp{<} and @samp{>}
 operators sort lexicographically using ASCII ordering.
 
diff --git a/test.c b/test.c
index 2b12197a..e16337a5 100644
--- a/test.c
+++ b/test.c
@@ -476,24 +476,6 @@ unary_operator (void)
   if (test_unop (op) == 0)
     return (FALSE);
 
-  /* the only tricky case is `-t', which may or may not take an argument. */
-  if (op[1] == 't')
-    {
-      advance (0);
-      if (pos < argc)
-       {
-         if (legal_number (argv[pos], &r))
-           {
-             advance (0);
-             return (unary_test (op, argv[pos - 1], 0));
-           }
-         else
-           return (FALSE);
-       }
-      else
-       return (unary_test (op, "1", 0));
-    }
-
   /* All of the unary operators take an argument, so we first call
      unary_advance (), which checks to make sure that there is an
      argument, and then advances pos right past it.  This means that
@@ -603,7 +585,7 @@ unary_test (char *op, char *arg, int flags)
 
     case 't':  /* File fd is a terminal? */
       if (legal_number (arg, &r) == 0)
-       return (FALSE);
+       integer_expected_error (arg);
       return ((r == (int)r) && isatty ((int)r));
 
     case 'n':                  /* True if arg has some length. */