vt: Fix and sync setting scrollback (history) docs
Needs ReviewPublic
Actions

Authored by ziaee on Sat, Oct 10, 2:36 PM.

Details

Reviewers

adrian
emaste
bcr

Group Reviewers

manpages

Summary

So, this was wrong in two places. The first one, I wrote, so... Sorry.
The second one... should we dig deeper? was the doc right initially?
Like does sc work differently than vt here and that is why?

MFC after: 3 days
Fixes: a8740ba860bf ("vt: Document increasing scrollback size")

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 78153
Build 75036: arc lint + arc unit

Event Timeline

ziaee created this revision.Sat, Oct 10, 2:36 PM

Herald added a subscriber: imp. · View Herald TranscriptSat, Oct 10, 2:36 PM

ziaee requested review of this revision.Sat, Oct 10, 2:36 PM

Harbormaster completed remote builds in B77961: Diff 189361.Sat, Oct 10, 2:36 PM

Good to go from manpages.

This revision is now accepted and ready to land.Sun, Oct 11, 9:02 AM

jilles added a subscriber: jilles.Sun, Oct 11, 2:17 PM

jilles added inline comments.

share/man/man5/rc.conf.5
3527	typo

Hmm, so I guess the real thing is you just want to document the maximum supported value? I think doing that in examples is not great actually. I would rather add a note to the description of the -h option in vt.8 describing the range of acceptable values and leave these examples alone unless they are describing invalid values (which the first example seems to be doing). So I would suggest dropping the change to rc.conf.5 entirely, tweaking the example in vt.8 to use a valid value and then add new text for the -h option to describe the range of accepted values.

share/man/man4/vt.4
364	"add" in the old text seems fine to me FWIW, "insert" would be a bit less colloquial than "put" perhaps?

Note that 16384 is not an intentionally set limit, it is just a consequence of an expression to ensure we don't have an integer overflow. It is somewhat conservative and could be made higher if there's a desire to do so.

Also the kernel returns EINVAL for values too large, but we could equally easily cap it at the maximum instead of rejecting values too high, if we want.

jilles: fixed thanks
jhb: rc.conf.5 is wrong, allscreens_kbdflags does not accept -h. we dont have vt.8, but i did add suggested default and maximum to vidcontrol.8, thanks. i dont want to leave it at just that because vidocontrol is arcane and mostly doesnt work for vt.
emaste: 16384 is really unbeliably tiny. the default 100 is completely useless and can not even show a pkg upgrade, but that makes sense since people dont actually use the console generally

This revision now requires review to proceed.Tue, Oct 13, 9:24 PM

Harbormaster completed remote builds in B78153: Diff 189797.Tue, Oct 13, 9:24 PM

jhb added inline comments.Thu, Oct 15, 9:57 PM

share/man/man5/rc.conf.5
3522	Ah, if you are just moving the text to the right variable, I would keep the old text as is, especially if `-h 200` works. I think that's part of what makes this diff confusing is both the logical change of moving it to the right variable and the second change of changing the actual text. I would probably just do a single dedicated commit just for rc.conf.5 to move the existing text to the right spot with a log message of something like "the scroll back setting must be set in all screens_flags rather than allscreens_kbdflags". Note that your current log message isn't really clear at all as it doesn't say what is wrong and what you are fixing, leaving people to guess.

ziaee added inline comments.Fri, Oct 16, 11:04 PM

share/man/man5/rc.conf.5
3522	Sorry, I will improve the commit message. I do not want to leave it at 200 because: 200 is useless. What if you had only 200 lines of scrollback? 200 does not show the limit, which is lower than anyone would imagine.