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

[Jim Meyering]

Kamil Dudka wrote:
> 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...

Glad you remembered that.

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

Fine with me.

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

Good.  Thanks.  two of these: s/ group/ the group/
s/Now/Now,/

> "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?

I was thinking of a comment in the .m4 file,
but now that you mention the help string, perhaps that's better.
You choose.

>> > 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 :-)

;-)
You're right that we go to extremes to avoid in-functions #ifdefs.
In-function #ifdefs are evil, but so is code duplication.
It's a trade-off.  Since this function is so small,
and the fraction of duplicated code would have been so high,
the in-function #ifdef is clearly the lesser evil.