Re: grep-2.10 on OSF/1

[Eric Blake]

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?

> +# 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.

> +  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

> +    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".

> +  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

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.

> +  return 1
> +}

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
}

> +# 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|2) compare_ "$@";;
> +    1) return 1;;

Given my above suggestions about compare_dev_null_ return value, this
should be:

compare ()
{
  compare_dev_null_ "$@"
  case $? in
    0|1) return $?;;
    *) compare_ "$@";;
  esac
}

-- 
Eric Blake   address@hidden    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature