bug#9987: [PATCH] groups,id: add -0, --null option

[Bernhard Voelker]

> On September 19, 2013 at 3:31 AM Pádraig Brady <address@hidden> wrote:
> 
> On 09/19/2013 12:24 AM, Bernhard Voelker wrote:
> > diff --git a/NEWS b/NEWS

> > +  id accepts a new option: --zero (-z) to separate the output entries by
> > +  a NUL instead of a white space character.
> 
> s/separate/delimit/

done.

> > diff --git a/doc/coreutils.texi b/doc/coreutils.texi
> >address@hidden
> >address@hidden
> >address@hidden
> >address@hidden
> > +Separate output items with NUL characters.
> 
> s/Separate/Delimit/

Actually I copied that sentence from other sections.
I guess it's wrong there, too.  And probaby it makes
sense to replace it by a macro.

> >address@hidden
> > +$ id -Gn --zero
> > +users <NUL> devs
> 
> users <NUL> devs <NUL>

Good point!
 
> > diff --git a/tests/misc/id-zero.sh b/tests/misc/id-zero.sh
> 
> > +id root || fail=1
> 
> Is 'root' a guaranteed name?
> misc/chroot-credentials.sh since v7.4-16-gc45c51f assumes so.
> I'm still not convinced (and will look at addressing that separately).

Cygwin lacks a 'root' entry - but it also lacks getent ...

> > +# Create a nice list of users (ignoring errors) ...
> > +getent passwd | head -n 100 | cut -d: -f1 > users
> 
> test -s users || skip_ 'Failed to read the passwd database with getent'

... so this test is good; added.

> Also why so many (100). The test is a bit slow and you would get
> the same coverage from a couple of entries?

Actually that was to protect against "very-expensive" behavior on
bigger NIS installation. On normal installations, I haven't seen much
more than 30-40 users yet anyway.

The point was to get an entry with only 1 and with 2 or more groups.
I wouldn't go that far and add require_membership_in_two_groups_,
so maybe finding such entries with something like this would be better
(btw: can we rely on xargs to be there)?

  getent passwd | head -n50 | cut -d: -f1 \
    | xargs groups \
    | awk '{ x[NF] = $1; }
           END { for (u in x) print x[u]; }'

Anyway, I moved the tr(1) out of the loop, so we're saving a few
pipe creations.

> > +# id(1) should refuse --zero in default format.
> > +id --zero > out 2>err && fail=1
> > +compare /dev/null out || fail=1
> 
> > +grep 'option --zero not permitted in default format' err || fail=1
> 
> Better to:
> echo 'option ...' > exp-err
> compare exp-err err || fail=1

true, changed.
 
> > +printf "\n" > exp || framework_failure_
> 
> Minor point, but it's better to use '' quotes when not interpolating,
> here and throughout the test.

true, thanks.

> > +cp /dev/null  out || framework_failure_
> 
> :> out

yep, done.

Thanks for the review.
I'll provide a new version of the patch later (or tomorrow).

Have a nice day,
Berny