It's nice to be able to display a full IPv6 host address if
needed, but it's also nice to display more than 3 characters of a command
line. Compute the needed size for the FROM column in an earlier pass,
and determine the maximum, then print what fits for the command.
Details
Details
tested on amd64
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
@cem: Not if it is an improvement on another change, and makes better use of the existing space.
| usr.bin/w/w.c | ||
|---|---|---|
| 99 ↗ | (On Diff #60641) | I think this is an HTML rendering artifact; it's right in the code. |
| 337 ↗ | (On Diff #60641) | Yes, and it was wrong (fromwidth wasn't computed with -h). I'm moving it, but it makes it look like fromwidth is used before set (but it is not). |
Comment Actions
@cem: phabricator says "cem requested changes to this revision.", but I see no changes requested.
Comment Actions
Sorry that this was unclear. There is a limitation in phabricator to the kinds of negative feedback metadata one can provide. It's "request changes" or nothing. So I use "request changes" here to explicitly mark "not approved."
In this case, the concern is the change breaks common interactive uses and POLA of an ancient BSD command to work around user error. It adds even more magic to how column widths are computed when there should probably be less.
One less magical option would be to do something like Linux' procps-ng w and have environment variables to control the width of at least a subset of columns. w.1 on Linux documents PROCPS_USERLEN and PROCPS_FROMLEN environment variable controls. Something similar in this vein would be to make the desired magical behavior conditional on a flag, such as -m for Magical.
A high-level option would be to add support for the environment variable COLUMNS or flag(s) -ww to w rather than magical inference from TTY parameters. This would allow users stuck on 80-column terminals to view the full command with a scrolling pager, like the less we've shipped since May 2000.
| usr.bin/w/w.c | ||
|---|---|---|
| 99 ↗ | (On Diff #60649) | Obligatory "fix the POLA" change-request comment in case it isn't clear from the "requests changes" annotation. |
| 322–323 ↗ | (On Diff #60649) | I agree with markm that macros that act on local variables should take them as explicit parameters rather than implicit references by name. E.g., #define WUSED(fromwidth) (W_DISPUSERSIZE + (fromwidth) + ... ) |
| 353–408 ↗ | (On Diff #60649) | As long as we're moving this code, this is a fantastic time to create a subroutine instead of just pasting it bodily into the other function. This is exactly the kind and scale of code where a subroutine with specific intent helps improve clarity. |
Comment Actions
Sorry that this was unclear. There is a limitation in phabricator to the kinds of negative feedback metadata one can provide. It's "request changes" or nothing.
Maybe there is a reason for that.
In this case, the concern is the change breaks common interactive uses and POLA of an ancient BSD command to work around user error. It adds even more magic to how column widths are computed when there should probably be less.
I see no breakage or POLA here. I don't know what possible user error you are talking about; the error of creating a fixed-width field for something that rarely happens? There is no magic to computing the required width of a column, and BSD programs of this sort have been doing this for at least 30 years (see ls and column, which derives from it).
One less magical option would be to do something like Linux' procps-ng w and have environment variables to control the width of at least a subset of columns. w.1 on Linux documents PROCPS_USERLEN and PROCPS_FROMLEN environment variable controls. Something similar in this vein would be to make the desired magical behavior conditional on a flag, such as -m for Magical.
I won't bother to check how this Linux program works. Environment variables can't compute the required width of a column. Do we set PS_WIDTH_PID, PS_WIDTH_PPID, PS_WIDTH_UID, etc? Of course not.
A high-level option would be to add support for the environment variable COLUMNS or flag(s) -ww to w rather than magical inference from TTY parameters. This would allow users stuck on 80-column terminals to view the full command with a scrolling pager, like the less we've shipped since May 2000.
I don't know where this comes from. I did not change the overall line length, and I don't see that using TTY screen size is any more magical now than when added 30 years ago.
| usr.bin/w/w.c | ||
|---|---|---|
| 322–323 ↗ | (On Diff #60649) | Mark didn't say that, but OK. |
| 353–408 ↗ | (On Diff #60649) | I don't agree. The code goes within an existing loop to set up an entry, and I see no point to turning it into a subroutine. |
Comment Actions
Let me go back to the beginning in an attempt to unravel this. Let's start with Conrad's question:
"Isn't changing the output of w a POLA violation?"
Conrad: Please provide a concrete example of how you think this change violates POLA and why?
Comment Actions
Conrad: it has now been more than a week since Marcel asked a question. If you can't or won't explain why you oppose this change, I think you need to drop your block on the review.