[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] who: --mesg now respects also group of a TTY
From: |
Jim Meyering |
Subject: |
Re: [PATCH] who: --mesg now respects also group of a TTY |
Date: |
Thu, 21 Jan 2010 13:04:27 +0100 |
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.
- [PATCH] who: --mesg now respects also group of a TTY, Kamil Dudka, 2010/01/20
- Re: [PATCH] who: --mesg now respects also group of a TTY, Jim Meyering, 2010/01/21
- Re: [PATCH] who: --mesg now respects also group of a TTY, Kamil Dudka, 2010/01/21
- Re: [PATCH] who: --mesg now respects also group of a TTY,
Jim Meyering <=
- [PATCH v2] who --mesg now checks the group of TTY devices, Kamil Dudka, 2010/01/22
- Re: [PATCH v2] who --mesg now checks the group of TTY devices, Jim Meyering, 2010/01/22
- Re: [PATCH v2] who --mesg now checks the group of TTY devices, Kamil Dudka, 2010/01/22
- Re: [PATCH v2] who --mesg now checks the group of TTY devices, Kamil Dudka, 2010/01/22
- Re: [PATCH v2] who --mesg now checks the group of TTY devices, Kamil Dudka, 2010/01/23
- Re: [PATCH v2] who --mesg now checks the group of TTY devices, Jim Meyering, 2010/01/25