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
