Re: init.sh compare function

[Jim Meyering]

Bruno Haible wrote:
> [dropping bug-grep from CC]
>
>> > --- 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.
>
> I like the idea, but as a person who often looks at failing tests and
> interprets the output I have two comments:
>
> 1) I would find it useful if, despite recognizing /dev/null as a special
>    case, the output format would stay the same or nearly the same.
>    When I see a line "+foo bar" or "-foo bar", possibly preceded by a diff
>    hunk, I know that the program produced or did not produce a line
>    containing "foo bar". Whereas when I see a line "foo bar" I think of
>    output that went to stdout of stderr and which should have been piped
>    away.

Good point.  I will do this as a separate change.  Patch below.

> 2) I would also find it useful to mention in comments that compare
>    function is meant to be called as in
>       compare expected_output actual_output
>    and not the opposite.
>
>    Why? Because a "+" indicates something that was added, whereas "-"
>    indicates something that was removed or missing. Most often the
>    program's actual output is wrong, not the expected output.
>    If you call
>       compare actual_output expected_output
>    then extraneous lines come out as "-line" and omitted ones come out as
>    "+line", which is counter-intuitive.
>    Whereas if you call
>       compare expected_output actual_output
>    then extraneous lines come out as "+line" and omitted ones come out as
>    "-line".
>
>    Yes I know it would take some work to revisit all coreutils and grep
>    tests that use 'compare', but that is not an urgent task.

I agree.  I prefer it that way, too.

For example, this makes more sense to me:

    good$ echo unexpected | diff -u /dev/null -
    --- /dev/null   2011-10-27
    +++ -   2011-11-22
    @@ -0,0 +1 @@
    +unexpected

than this:

    bad$ echo unexpected | diff -u - /dev/null
    --- -   2011-11-22
    +++ /dev/null   2011-10-27
    @@ -1 +0,0 @@
    -unexpected

I was surprised to find that the vast majority of "compare"
uses in coreutils are reversed.  I.e., "compare exp out" is
far more common than "compare out exp":

    $ git grep 'compare exp'|wc -l
    28
    $ git grep -h -E 'compare [^ ]+ exp'|wc -l
    212

I guess it's because I rarely (never?) see output from them.

These too are reversed:

    $ git grep -E -h 'compare [^ ]+ /dev/null'
    compare err /dev/null || fail=1
    compare err /dev/null || fail=1
    compare err /dev/null || fail=1

The good news is that it's easy to fix them mechanically, e.g.,

    git grep -l -E 'compare [^ ]+ exp' \
      |xargs perl -pi -e 's/(compare) (\S+) (exp\S*)/$1 $3 $2/'

so I've done that in coreutils:

    http://git.sv.gnu.org/cgit/coreutils.git/commit/?id=a2c811db420717d61b

>> > +# 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
>
> So here I would emit a fake hunk header and then either
>    sed 's/^/+/' "$1"
> or
>    sed 's/^/-/' "$1"

I too wanted that, but it was late.  Thanks for the prod.

With the init.sh patch below, and with this deliberately-introduced
error in coreutils:

diff --git a/tests/du/max-depth b/tests/du/max-depth
index e165d32..a6265eb 100755
--- a/tests/du/max-depth
+++ b/tests/du/max-depth
@@ -26,6 +26,7 @@ du --max-depth=2 a > out 2>err || fail=1
 # Remove the sizes.  They vary between file systems.
 cut -f2- out > k && mv k out
 compare exp out || fail=1
+echo foo > err
 compare /dev/null err || fail=1

 # Repeat, but use -d 1.

When I run "make check -C tests TESTS=du/max-depth VERBOSE=yes",
now I see this in the .log file:

    + compare /dev/null err
    + compare_dev_null_ /dev/null err
    + test 2 = 2
    + test x/dev/null = x/dev/null
    + test -s err
    + emit_diff_u_header_ /dev/null err
    + printf '%s\n' 'diff -u /dev/null err' '--- /dev/null  1970-01-01' '+++ 
err    1970-01-01'
    diff -u /dev/null err
    --- /dev/null   1970-01-01
    +++ err 1970-01-01
    + sed 's/^/+/' -- err
    +foo

It's not perfect, but better than what we had before.

Here's an additional change for gnulib's init.sh, as you suggested:

>From 67c0ae987b8c4aec0680edbeb81096471db4fb8f Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Tue, 22 Nov 2011 14:51:45 +0100
Subject: [PATCH] init.sh: make "compare /dev/null FILE" output more readable

* tests/init.sh (compare_): Document the preferred order of arguments.
(emit_diff_u_header_): New function.
(compare_dev_null_): Emit a simulated diff, rather than just the
contents of the unexpected file.  Suggestion from Bruno Haible.
---
 ChangeLog     |    8 ++++++++
 tests/init.sh |   28 ++++++++++++++++++++--------
 2 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 0fbcf89..b9e5d59 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2011-11-22  Jim Meyering  <address@hidden>
+
+       init.sh: make "compare /dev/null FILE" output more readable
+       * tests/init.sh (compare_): Document the preferred order of arguments.
+       (emit_diff_u_header_): New function.
+       (compare_dev_null_): Emit a simulated diff, rather than just the
+       contents of the unexpected file.  Suggestion from Bruno Haible.
+
 2011-11-21  Jim Meyering  <address@hidden>
            Eric Blake  <address@hidden>

diff --git a/tests/init.sh b/tests/init.sh
index 5079010..e2f6119 100644
--- a/tests/init.sh
+++ b/tests/init.sh
@@ -221,6 +221,15 @@ export MALLOC_PERTURB_
 # a partition, or to undo any other global state changes.
 cleanup_ () { :; }

+# Emit a header similar to that from diff -u;  Print the simulated "diff"
+# command so that the order of arguments is clear.  Don't bother with @@ lines.
+emit_diff_u_header_ ()
+{
+  printf '%s\n' "diff -u $*" \
+    "--- $1    1970-01-01" \
+    "+++ $2    1970-01-01"
+}
+
 # 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, or no argument is /dev/null, return 2.
@@ -232,17 +241,18 @@ compare_dev_null_ ()
   test $# = 2 || return 2

   if test "x$1" = x/dev/null; then
-    set dummy "$2" "$1"; shift
+    test -s "$2" || return 0
+    { emit_diff_u_header_ "$@"; sed 's/^/+/' -- "$2"; } >&2
+    return 1
   fi

-  test "x$2" = x/dev/null || return 2
-
-  test -s "$1" || return 0
+  if test "x$2" = x/dev/null; then
+    test -s "$1" || return 0
+    { emit_diff_u_header_ "$@"; sed 's/^/-/' -- "$1"; } >&2
+    return 1
+  fi

-  cat - "$1" <<EOF >&2
-Unexpected contents of $1:
-EOF
-  return 1
+  return 2
 }

 if diff_out_=`( diff -u "$0" "$0" < /dev/null ) 2>/dev/null`; then
@@ -288,6 +298,8 @@ else
   compare_ () { cmp "$@"; }
 fi

+# Usage: compare EXPECTED ACTUAL
+#
 # Given compare_dev_null_'s preprocessing, defer to compare_ if 2 or more.
 # Otherwise, propagate $? to caller: any diffs have already been printed.
 compare ()
--
1.7.8.rc3.23.ge14d6