Re: grep-2.10 on OSF/1

[Jim Meyering]

Eric Blake wrote:
> On 11/21/2011 01:53 PM, Jim Meyering wrote:
>>>> That is, rather than creating an empty errexp, I'd rather see this test
>>>> rewritten to use a form of test -s.
>>>
>>> How about having compare "know" about /dev/null.
>>> Then it can perform the test -s and warn if the file is nonempty.
>>> With that, all existing (and there are many) /dev/null-using
>>> compare uses will benefit.
>
> Nice idea.
>
>>
>> I've implemented that.
>> What do you think?
>
> Comments below.  Also, are you planning on pushing this back up to
> gnulib's init.sh?

Yes.  In fact, c-set was against gnulib's init.sh
(hence included ChangeLog diffs)

>> +# Arrange not to let diff or cmp operate on /dev/null,
>> +# since on some systems (at least OSF/1 5.1), that doesn't work.
>> +# When there are not two arguments, return 2.
>> +# When one argument is /dev/null and the other is not empty,
>> +# cat the nonempty file to stderr and return 1.
>> +# Otherwise, return 0.
>
> That return convention is hard to use - 0 means both: one file was
> /dev/null and the other is empty, so comparison is successful, and
> neither file was /dev/null so comparison is indeterminate.  Rather, you
> should return 2 if neither file is /dev/null, so that you need not call
> compare_() if the return is 0 (otherwise, your patch is worthless - you
> still end up triggering the bogus OSF/1 comparison to /dev/null).
>
>> +compare_dev_null_ ()
>> +{
>> +  local err
>
> 'local' is not POSIX; you are likely to run into a shell that can't
> handle it.

Good catch.
I have to remember that in tests/, init.sh is the sole script
in which I cannot use "local".

>> +  test $# = 2 || return 2
>> +
>> +  if test "$1" = /dev/null; then
>
> Not portable if $1 is '(' or begins with '-'; you need:
>
> if test "x$1" = x/dev/null; then

Yep.  Shame on me.

>> +    test -s "$2" && err="$2"
>
> If someone does 'compare /dev/null /dev/null', for whatever reason, then
> I'm hoping that 'test -s /dev/null' works on OSF/1 5.1? (not having
> access to that platform to test it myself).  But hopefully that's not a
> show-stopper.
>
> It is sufficient to write err=$2, rather than err="$2".

Yes, and I even prefer to omit the double quotes.
Double shame on me ;-)

>> +  elif test "$2" = /dev/null; then
>
> Same thing about needing 'x' to prevent test from mis-parsing arguments.
>
>> +    test -s "$1" && err="$1"
>> +  fi
>> +  test -z "$err" && return 0
>
> test -z "$err" is not portable if $err is '='; alas, portability demands
> this be written as

Hmm... I've probably made that mistake elsewhere, too.
Thanks.

> test "x$err" = x && return 0
>
>> +
>> +  cat "$err" 1>&2
>
> This doesn't identify the file being printed, which is less information
> than what 'diff -u "$err" /dev/null' provides on systems with working diff.

Good point.

> Since you can't portably use local, and given my concerns about the
> return value and identifying the problematic file, how about:
>
> compare_dev_null_ ()
> {
>   test $# = 2 || return 2
>
>   if test "x$1" = x/dev/null; then
>     set dummy "$2" "$1"; shift
>   fi
>
>   test "x$2" = x/dev/null || return 2
>
>   test -s "$1" || return 0
>
>   cat - "$1" <<EOF >&2
> Unexpected contents of $1:
>   EOF
>   return 1
> }

Used verbatim below, except that the EOF is not indented.

> Given my above suggestions about compare_dev_null_ return value, this
> should be:
>
> compare ()
> {
>   compare_dev_null_ "$@"
>   case $? in
>     0|1) return $?;;
>     *) compare_ "$@";;
>   esac
> }

I prefer that, too.
Thanks for the thorough review.
Here's the proposed gnulib commit.

>From e636d67f6ff4116312c789d4eec2af53b9cd7cd9 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Mon, 21 Nov 2011 21:50:23 +0100
Subject: [PATCH] init.sh: work around OSF/1 5.1's mishandling of /dev/null

* tests/init.sh: Make our compare function slightly more portable.
Reported by Bruno Haible in
http://thread.gmane.org/gmane.comp.gnu.grep.bugs/4020
Much improved by Eric Blake.
---
 ChangeLog     |    8 ++++++++
 tests/init.sh |   47 +++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 49 insertions(+), 6 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index e775587..0fbcf89 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2011-11-21  Jim Meyering  <address@hidden>
+           Eric Blake  <address@hidden>
+
+       init.sh: work around OSF/1 5.1's mishandling of /dev/null
+       * tests/init.sh: Make our compare function slightly more portable.
+       Reported by Bruno Haible in
+       http://thread.gmane.org/gmane.comp.gnu.grep.bugs/4020
+
 2011-11-21  Simon Josefsson  <address@hidden>

        * m4/gnulib-common.m4 (_Noreturn): Check that _MSC_VER is defined
diff --git a/tests/init.sh b/tests/init.sh
index c78e5b6..68e1f4f 100644
--- a/tests/init.sh
+++ b/tests/init.sh
@@ -221,11 +221,35 @@ export MALLOC_PERTURB_
 # a partition, or to undo any other global state changes.
 cleanup_ () { :; }

+# Arrange not to let diff or cmp operate on /dev/null,
+# since on some systems (at least OSF/1 5.1), that doesn't work.
+# When there are not two arguments, return 2.
+# When one argument is /dev/null and the other is not empty,
+# cat the nonempty file to stderr and return 1.
+# Otherwise, return 0.
+compare_dev_null_ ()
+{
+  test $# = 2 || return 2
+
+  if test "x$1" = x/dev/null; then
+    set dummy "$2" "$1"; shift
+  fi
+
+  test "x$2" = x/dev/null || return 2
+
+  test -s "$1" || return 0
+
+  cat - "$1" <<EOF >&2
+Unexpected contents of $1:
+EOF
+  return 1
+}
+
 if diff_out_=`( diff -u "$0" "$0" < /dev/null ) 2>/dev/null`; then
   if test -z "$diff_out_"; then
-    compare () { diff -u "$@"; }
+    compare_ () { diff -u "$@"; }
   else
-    compare ()
+    compare_ ()
     {
       if diff -u "$@" > diff.out; then
         # No differences were found, but Solaris 'diff' produces output
@@ -241,9 +265,9 @@ if diff_out_=`( diff -u "$0" "$0" < /dev/null ) 
2>/dev/null`; then
   fi
 elif diff_out_=`( diff -c "$0" "$0" < /dev/null ) 2>/dev/null`; then
   if test -z "$diff_out_"; then
-    compare () { diff -c "$@"; }
+    compare_ () { diff -c "$@"; }
   else
-    compare ()
+    compare_ ()
     {
       if diff -c "$@" > diff.out; then
         # No differences were found, but AIX and HP-UX 'diff' produce output
@@ -259,11 +283,22 @@ elif diff_out_=`( diff -c "$0" "$0" < /dev/null ) 
2>/dev/null`; then
     }
   fi
 elif ( cmp --version < /dev/null 2>&1 | grep GNU ) > /dev/null 2>&1; then
-  compare () { cmp -s "$@"; }
+  compare_ () { cmp -s "$@"; }
 else
-  compare () { cmp "$@"; }
+  compare_ () { cmp "$@"; }
 fi

+# Given compare_dev_null_'s preprocessing, for 0 or 2, defer to compare_.
+# Otherwise, differences have already been printed, so return 1.
+compare ()
+{
+  compare_dev_null_ "$@"
+  case $? in
+    0|1) return $?;;
+    *) compare_ "$@";;
+  esac
+}
+
 # An arbitrary prefix to help distinguish test directories.
 testdir_prefix_ () { printf gt; }

--
1.7.8.rc2.3.g0911