Re: [PATCH] who: --mesg now respects also group of a TTY

[Kamil Dudka]

Hi Jim,

thanks for review!

On Thursday 21 of January 2010 12:28:03 Jim Meyering wrote:
> Actually, more comments (in the code and log) would be welcome, too ;-)

Sure.  I also forgot the credit Piotr Gackiewicz as the original author
in the first place...

> Please add a little explanation before the new function, is_tty_writable

What about "Return true if a terminal device given as PSTAT allows other users 
to send messages to; false otherwise" ?

> and list one or both of those URLs for context in the commit log.

I think the rhbz link should be sufficient.

> > +** New features
> > +
> > +  who --mesg now respects also group of a TTY when compiled with
> > +  --with-tty-group
>
> Saying it "respects the group..." doesn't really communicate what is
> changed.  How about saying that it works now in some cases (details?)
> where it used to fail?

What about this?

"who --mesg used to ignore group of a TTY device when checking if it is 
possible to send messages there.  Now if coreutils is compiled 
with --with-tty-group[=TTY_GROUP_NAME] configure option, it also compares 
group of the TTY device with TTY_GROUP_NAME (or "tty" if no group is 
specified)."

> It would be helpful to say how to determine the appropriate group name.
> Something like "ls -lg /dev/tty" or
>     $ stat --format %G /dev/tty
>     tty

Do you mean directly in the help string or somewhere else?

>
> > diff --git a/src/who.c b/src/who.c
>
> ...
>
> There is enough duplication below that
> I'd prefer to move the #ifdef/#endif into the function.

I am all for that.  I thought it was prohibited in GNU coreutils :-)

> Also, please humor me and put the "const" after the type name:

np

Kamil