Re: Found and fixed bug in groups(1). Want the diff?

[Jim Meyering]

Iain Calder <address@hidden> wrote:
> OS:      GNU/Linux Debian 3.1 (sarge)
> Command: /usr/bin/groups
> Problem: Doesn't return failure when unable to display output.
> Notes:   groups(1) is a small shell script; 42 lines excluding
>          the copyright comments.
>
> The author went to great lengths to return failure when the
> program is unable to display output, but didn't do so on the main
> code path.  Thus the program's behaviour is inconsistent.  Witness:
>
>    $ groups --version; echo $?
>    groups (GNU coreutils) 5.2.1
>    0
>    $ groups --version >&-; echo $?
>    1                   <--- echo failure reflected in exit status
>
>    $ groups ic; echo $?
>    ic : ic dialout cdrom floppy audio src video plugdev
>    0
>    $ groups ic >&-; echo $?
>    0                   <--- echo failed, yet status is successful!
>
> The code was obfuscated.  I fixed the bug, made the script
> shorter as well as cleaner, then added a couple of comments.
> Is this code being maintained?  To whom should I send my diffs?

With no diffs, I can only guess, but here's a small additional change:

2006-09-26  Jim Meyering  <address@hidden>

        * src/groups.sh: When invoked with 0 or 1 argument, just exec "id".
        Rewrite to avoid using temporary, $status.

Index: src/groups.sh
===================================================================
RCS file: /fetish/cu/src/groups.sh,v
retrieving revision 1.20
diff -u -r1.20 groups.sh
--- src/groups.sh       26 Sep 2006 09:28:18 -0000      1.20
+++ src/groups.sh       26 Sep 2006 09:42:06 -0000
@@ -53,18 +53,17 @@
   * ) ;;
 esac

-if [ $# -eq 0 ]; then
-  id -Gn
-  fail=$?
-else
-  for name in "$@"; do
-    groups=`id -Gn -- $name`
-    status=$?
-    if test $status = 0; then
-      echo $name : $groups || fail=1
-    else
-      fail=$status
-    fi
-  done
-fi
+# With fewer than two arguments, simply exec "id".
+case $# in
+  0|1) exec id -Gn "$@" ;;
+esac
+
+# With more, we need a loop, and be sure to exit nonzero upon failure.
+for name in "$@"; do
+  if groups=`id -Gn -- $name`; then
+    echo $name : $groups || fail=1
+  else
+    fail=1
+  fi
+done
 exit $fail