On 2025-07-28 21:39, Collin Funk wrote:
Thanks for again for the thorough review and explanations. I find it
funny that I assumed this change was simple and learned it's purpose was
for logging in on a teletype. The Model 37 predates me ~30 years, so it
never occured to me that the purpose was to not print logins.
Heh, in the 1960s I used a Teletype Model 33, which was so bad that
nobody ever wanted to print anything nice on it and 'pr' would have
never gotten these flags if all people had were Model 33s. And that
wasn't the worst device I used to write programs!
but I digress....
+ pr now supports the -p option, to pause upon printing each page until
+ a newline character is read from /dev/tty, as required by POSIX. The
+ corresponding long option is --pause.
Change "upon" to "before".
+After printing each page, print an alert (bell) to standard error and
+wait for a newline to be read from @file{/dev/tty} before printing the
+next page.
This sentence should start "Before" not "After", with the rest of the
sentence changed accordingly. There is no bell printed after the last page.
Looking at the code, it seems that this mistake has leaked into the code
as well, in that sometimes 'pr' will print an alert and wait for '\n' at
end of file, when there is no page to print. If I'm right, that needs to
be fixed; pr should wait for '\n' only if it will actually print
something afterward.
+@vindex POSIXLY_CORRECT
+If the @env{POSIXLY_CORRECT} environment variable is set and standard
+output is not a tty, this option will be ignored.
Better wording is to start "This option is ignored if...". And change
"not a tty" to "not associated with a terminal". Similarly for the other
"not a tty"s. (Few people these days know that "tty" is short for
"Teletype"....)
+/* If true, pause after each page until a newline is read from /dev/tty. */
"after" -> "before"
+static bool pause_option;
+
+/* If true, pause on only the first page. */
The "only" sounds wrong, if both flags are set. I would omit "only".
Also, change "on" to "before".
+ /* POSIX states that the pausing behavior of -f and -p should only occur if
"should only occur" -> "should occur only"
+ if (pause_option && close (tty_fd) < 0)
+ error (EXIT_FAILURE, errno, "%s", quotef ("/dev/tty"));
Why are these lines useful? As far as I can see they merely add
complexity for no benefit. How about removing them? (If we kept them we
would need to fix the bug in them; but let's remove them.)
+ unsigned char ch;
Make it plain char; there's no reason it needs to be unsigned.
+ if (putc ('\a', stderr) == EOF || fflush (stderr) != 0)
fputc here; there's no need for efficiency since we're gonna fflush anyway.
More important, this fputc and fflush should be skipped if pause_option
is false.
You might want to turn this part of the code into a separate static
function, for clarity. (Or you might not....)
I assumed this change was simple
And you were right: it will be simple, once we get it done right....
